Skip to content

feat(skills-next): add dotnet SDK references#240

Open
evanpurkhiser wants to merge 1 commit into
mainfrom
evanpurkhiser/feat-skills-next-add-dotnet-sdk-references
Open

feat(skills-next): add dotnet SDK references#240
evanpurkhiser wants to merge 1 commit into
mainfrom
evanpurkhiser/feat-skills-next-add-dotnet-sdk-references

Conversation

@evanpurkhiser

Copy link
Copy Markdown
Member

Direct LLM port of the existing dotnet SDK skill into the skills-next per-SDK reference layout under skills-next/references/sdks/dotnet/. This content has NOT been reviewed at all — it is a machine-generated port of the existing SDK skill, and every file should be treated as unverified.

Comment on lines +407 to +408
var ctx = SentrySdk.ContinueTrace(envelope.SentryTrace, envelope.Baggage);
var transaction = SentrySdk.StartTransaction(ctx, "process-order", "function");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The documentation uses a three-argument SentrySdk.StartTransaction(ctx, "name", "op") overload that doesn't exist. The correct method takes a single TransactionContext argument.
Severity: MEDIUM

Suggested Fix

Update the code examples to use the correct single-argument overload SentrySdk.StartTransaction(ctx). The TransactionContext object ctx returned from SentrySdk.ContinueTrace already contains the necessary transaction name and operation.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: skills-next/references/sdks/dotnet/tracing.md#L407-L408

Potential issue: The documentation for .NET tracing contains incorrect code examples for
`SentrySdk.StartTransaction`. In the consumer queue example and the quick reference
cheat sheet, it shows a three-argument overload `SentrySdk.StartTransaction(ctx, "name",
"op")`. However, the Sentry .NET SDK does not have this overload. The correct usage,
shown elsewhere in the document, is to pass a single `TransactionContext` object, like
`SentrySdk.StartTransaction(ctx)`. Developers copying the incorrect examples will
encounter a compile error.

Also affects:

  • skills-next/references/sdks/dotnet/tracing.md:844~845

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's right
We do not expose an overload that has three string parameters.
There is an overload, thought, that takes a ITransactionContext and then two string.

Comment on lines +578 to +579
| Stack traces show no file/line | PDB files not uploaded | Add `SentryUploadSymbols=true` to `.csproj`; set `SENTRY_AUTH_TOKEN` in CI |
| WPF/WinForms exceptions missing | `IsGlobalModeEnabled` not set | Set `options.IsGlobalModeEnabled = true` in `SentrySdk.Init()` |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The index.md file contains incorrect relative paths to documentation files, which will likely cause them to fail to load.
Severity: MEDIUM

Suggested Fix

Update the paths in index.md to correctly point to the documentation files. The paths should be relative to the index.md file's location, such as error-monitoring.md, or use a variable like ${SKILL_ROOT} if the system supports it.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: skills-next/references/sdks/dotnet/index.md#L578-L579

Potential issue: The `index.md` file uses hardcoded relative paths to link to other
documentation files, such as `references/error-monitoring.md`. However, the actual files
are located at a different path, `references/sdks/dotnet/error-monitoring.md`. These
incorrect paths will likely prevent the AI system that consumes these files from loading
the reference documents, as the links will not resolve correctly within the project's
file structure.

Did we get this right? 👍 / 👎 to inform future reviews.

@dingsdax dingsdax requested a review from Flash0ver July 6, 2026 08:50
Comment on lines +83 to +94
| Type | C# keyword | Supported |
|------|-----------|-----------|
| `System.Byte` | `byte` | ✅ Yes |
| `System.Int16` | `short` | ✅ Yes |
| `System.Int32` | `int` | ✅ Yes |
| `System.Int64` | `long` | ✅ Yes |
| `System.Single` | `float` | ✅ Yes |
| `System.Double` | `double` | ✅ Yes |
| `System.UInt32` | `uint` | ❌ No |
| `System.UInt64` | `ulong` | ❌ No |
| `System.Decimal` | `decimal` | ❌ No |
| `System.Int128` | `Int128` | ❌ No |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: focus on types with keyword

This list is a bit "incomplete" / "inconsistent":

  • It does not mention all integral types with a keyword: sbyte, ushort, nint, nuint
  • It mentions only one integral type without a keyword (Int128), but what about System.UInt128, and System.Half (a floating-point type without a keyword).

I suggest mentioning all numeric types that have a keyword, but excluding native integers, so we list the most common numeric types.

Suggested change
| Type | C# keyword | Supported |
|------|-----------|-----------|
| `System.Byte` | `byte` | ✅ Yes |
| `System.Int16` | `short` | ✅ Yes |
| `System.Int32` | `int` | ✅ Yes |
| `System.Int64` | `long` | ✅ Yes |
| `System.Single` | `float` | ✅ Yes |
| `System.Double` | `double` | ✅ Yes |
| `System.UInt32` | `uint` | ❌ No |
| `System.UInt64` | `ulong` | ❌ No |
| `System.Decimal` | `decimal` | ❌ No |
| `System.Int128` | `Int128` | ❌ No |
| Type | C# keyword | Supported |
|------|-----------|-----------|
| `System.Byte` | `byte` | ✅ Yes |
| `System.Int16` | `short` | ✅ Yes |
| `System.Int32` | `int` | ✅ Yes |
| `System.Int64` | `long` | ✅ Yes |
| `System.Single` | `float` | ✅ Yes |
| `System.Double` | `double` | ✅ Yes |
| `System.SByte` | `sbyte` | ❌ No |
| `System.UInt16` | `ushort` | ❌ No |
| `System.UInt32` | `uint` | ❌ No |
| `System.UInt64` | `ulong` | ❌ No |
| `System.Decimal` | `decimal` | ❌ No |
  • removes Int128
  • adds System.SByte
  • adds System.UInt16

Comment on lines +19 to +26
// Signature
SentryId CaptureCheckIn(
string monitorSlug,
CheckInStatus status,
SentryId? checkInId = null,
TimeSpan? duration = null,
Action<SentryMonitorOptions>? configureMonitorOptions = null
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: API is wrong

Suggested change
// Signature
SentryId CaptureCheckIn(
string monitorSlug,
CheckInStatus status,
SentryId? checkInId = null,
TimeSpan? duration = null,
Action<SentryMonitorOptions>? configureMonitorOptions = null
)
// Signature
SentryId CaptureCheckIn(
string monitorSlug,
CheckInStatus status,
SentryId? sentryId = null,
TimeSpan? duration = null,
Scope? scope = null,
Action<SentryMonitorOptions>? configureMonitorOptions = null
)
  • parameter SentryId has wrong name
  • parameter Scope missing

Comment on lines +110 to +115
options.Schedule = "0 2 * * *"; // 2 AM daily (crontab expression)
options.CheckInMargin = 5; // 5 min grace period before "missed"
options.MaxRuntime = 30; // alert if running longer than 30 min
options.TimeZone = "America/New_York"; // IANA timezone
options.FailureIssueThreshold = 2; // create issue after 2 consecutive failures
options.RecoveryThreshold = 1; // resolve issue after 1 consecutive success

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: example does not compile

Suggested change
options.Schedule = "0 2 * * *"; // 2 AM daily (crontab expression)
options.CheckInMargin = 5; // 5 min grace period before "missed"
options.MaxRuntime = 30; // alert if running longer than 30 min
options.TimeZone = "America/New_York"; // IANA timezone
options.FailureIssueThreshold = 2; // create issue after 2 consecutive failures
options.RecoveryThreshold = 1; // resolve issue after 1 consecutive success
options.Interval("0 2 * * *"); // 2 AM daily (crontab expression)
options.CheckInMargin = TimeSpan.FromMinutes(5); // 5 min grace period before "missed"
options.MaxRuntime = TimeSpan.FromMinutes(30); // alert if running longer than 30 min
options.TimeZone = "America/New_York"; // IANA timezone
options.FailureIssueThreshold = 2; // create issue after 2 consecutive failures
options.RecoveryThreshold = 1; // resolve issue after 1 consecutive success
  • property Schedule does not exist ... use method Interval instead
  • property CheckInMargin is a TimeSpan, not an int
  • property MaxRuntime is a TimeSpan, not an int

Comment on lines +128 to +133
options.Interval(6, SentryMonitorInterval.Hour); // every 6 hours
options.CheckInMargin = 30;
options.MaxRuntime = 120;
options.TimeZone = "UTC";
options.FailureIssueThreshold = 1;
options.RecoveryThreshold = 3;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: does not compile

Suggested change
options.Interval(6, SentryMonitorInterval.Hour); // every 6 hours
options.CheckInMargin = 30;
options.MaxRuntime = 120;
options.TimeZone = "UTC";
options.FailureIssueThreshold = 1;
options.RecoveryThreshold = 3;
options.Interval(6, SentryMonitorInterval.Hour); // every 6 hours
options.CheckInMargin = TimeSpan.FromMinutes(30);
options.MaxRuntime = TimeSpan.FromMinutes(120);
options.TimeZone = "UTC";
options.FailureIssueThreshold = 1;
options.RecoveryThreshold = 3;
  • CheckInMargin and MaxRuntime are of type TimeSpan, not of type int

Comment on lines +155 to +156
| `Schedule` | `string` | Standard crontab expression (e.g., `"*/15 * * * *"`) |
| `Interval(n, unit)` | method | Interval-based schedule; alternative to `Schedule` |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: incorrect options

Suggested change
| `Schedule` | `string` | Standard crontab expression (e.g., `"*/15 * * * *"`) |
| `Interval(n, unit)` | method | Interval-based schedule; alternative to `Schedule` |
| `Interval(crontab)` | method | Standard crontab expression (e.g., `"*/15 * * * *"`) |
| `Interval(interval, unit)` | method | alternative interval-based schedule |
  • Schedule does not exist
  • name of first parameter to Interval is interval rather than n

Comment on lines +407 to +408
var ctx = SentrySdk.ContinueTrace(envelope.SentryTrace, envelope.Baggage);
var transaction = SentrySdk.StartTransaction(ctx, "process-order", "function");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's right
We do not expose an overload that has three string parameters.
There is an overload, thought, that takes a ITransactionContext and then two string.

);

var app = builder.Build();
app.UseSentry();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: again, there is no UseSentry extension method receiving an WebApplication


```csharp
// ❌ Loses exception details
activity.RecordException(ex);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: is this correct? @jamescrosswell


### Option 2: Bridge Pattern

> **Added in:** `Sentry` ≥6.1.0, `Sentry.OpenTelemetry` ≥6.1.0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: this is here correct? @jamescrosswell

Comment on lines +589 to +614
```csharp
var builder = WebApplication.CreateBuilder(args);

// Part 1: Configure Sentry with UseOpenTelemetry()
builder.WebHost.UseSentry(options =>
{
options.Dsn = "https://...@o0.ingest.sentry.io/...";
options.TracesSampleRate = 1.0;
options.UseOpenTelemetry(); // ← tells Sentry to use OTel for trace context propagation
// Do NOT also configure Sentry's own DiagnosticSource integration —
// let OTel instrumentation libraries handle it instead
});

// Part 2: Register OTel TracerProvider with AddSentry()
builder.Services.AddOpenTelemetry()
.WithTracing(tracing => tracing
.AddAspNetCoreInstrumentation()
.AddHttpClientInstrumentation()
.AddEntityFrameworkCoreInstrumentation()
.AddSentry() // ← converts OTel spans to Sentry format
);

var app = builder.Build();
app.UseSentry();
app.Run();
```

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: this code sample looks quite wrong @jamescrosswell

@Flash0ver Flash0ver left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I haven't checked the Sentry-JS-integration
  • I need a bit more time to review Profiling
  • I need a bit more time to review Tracing

@Flash0ver Flash0ver self-assigned this Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants