Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Tracing without performance #2493

Merged
merged 49 commits into from
Aug 3, 2023
Merged

feat: Tracing without performance #2493

merged 49 commits into from
Aug 3, 2023

Conversation

bitsandfoxes
Copy link
Contributor

@bitsandfoxes bitsandfoxes commented Jul 18, 2023

Resolves #2447
Implementations in other SDKs:

The idea here is:
The scope now has a SentryPropagationContext. The context contains minimally a traceID and a spanID. ContinueTrace overwrites the context on the current scope, making it possible to propagate the IDs without the performance feature enabled.
Relevant top-level methods: GetTraceHeader, GetBaggage. The methods first look for active spans before falling back to creating their return value from the PropagationContext. The same is true for CaptureEvent where we check for linked or active spans first.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 18, 2023

Fails
🚫 Please consider adding a changelog entry for the next release.
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Tracing without performance ([#2493](https://github.com/getsentry/sentry-dotnet/pull/2493))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 139579a

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Great work! left some notes

src/Sentry/BaggageHeader.cs Show resolved Hide resolved
src/Sentry/DynamicSamplingContext.cs Outdated Show resolved Hide resolved
src/Sentry/IHub.cs Outdated Show resolved Hide resolved
src/Sentry/IHub.cs Outdated Show resolved Hide resolved
src/Sentry/Internal/Hub.cs Outdated Show resolved Hide resolved
src/Sentry/Internal/Hub.cs Outdated Show resolved Hide resolved
src/Sentry/Internal/Hub.cs Outdated Show resolved Hide resolved
src/Sentry/Sentry.csproj Outdated Show resolved Hide resolved
src/Sentry/SentryPropagationContext.cs Outdated Show resolved Hide resolved
src/Sentry/SentrySdk.cs Outdated Show resolved Hide resolved
Copy link
Collaborator

@jamescrosswell jamescrosswell left a comment

Choose a reason for hiding this comment

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

Looking good! Not too much to add to the comments Bruno left already.

src/Sentry/Internal/Hub.cs Show resolved Hide resolved
src/Sentry/Internal/Hub.cs Show resolved Hide resolved
bitsandfoxes and others added 4 commits July 28, 2023 19:00
Co-authored-by: James Crosswell <jamescrosswell@users.noreply.github.com>
Copy link
Collaborator

@jamescrosswell jamescrosswell left a comment

Choose a reason for hiding this comment

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

I didn't do a full review - just the files that have changed since I last checked a few days ago. All looking good.

My only question is what we plan to do with the EnableTracing option.

Otherwise, once the Check failures from CI are addressed, I think it's looking good.

string? operation = null)
{
// Transactions from DisabledHub are always sampled out
return new TransactionContext(string.Empty, string.Empty, false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could do the same thing we've done for the DisabledHub.Instance here, to avoid creating a new object every time this method is called.

Not sure where to put that... maybe TransactionContext.Disabled or TransactionContext.DisabledInstance?

src/Sentry/SentryPropagationContext.cs Outdated Show resolved Hide resolved
@jamescrosswell
Copy link
Collaborator

A bit annoying, but you have to run a build on a Windows box to get the verify tests running (and the file you need to commit) to fix this:

image

CHANGELOG.md Outdated Show resolved Hide resolved
@bitsandfoxes bitsandfoxes merged commit 55ca61e into main Aug 3, 2023
15 of 16 checks passed
@bitsandfoxes bitsandfoxes deleted the feat/twp branch August 3, 2023 12:41
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.

Tracing without Performance
3 participants