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

Fixes: Identifying Users with OpenTelemetry doesn't work #2618

Merged
merged 10 commits into from Sep 18, 2023

Conversation

jamescrosswell
Copy link
Collaborator

@jamescrosswell jamescrosswell commented Sep 14, 2023

Fixes Identifying Users with OpenTelemetry doesn't work #2616.

Key implementation details

@jamescrosswell jamescrosswell linked an issue Sep 14, 2023 that may be closed by this pull request
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.

Awesome work! Happy you found a way around without having to introduce a new package or add AspNetCore as a dep to Sentry.OTel!!

src/Sentry.AspNetCore/IUserFactory.cs Outdated Show resolved Hide resolved
src/Sentry.OpenTelemetry/AspNetCoreEnricher.cs Outdated Show resolved Hide resolved
src/Sentry.OpenTelemetry/AspNetCoreEnricher.cs Outdated Show resolved Hide resolved
src/Sentry.OpenTelemetry/SentrySpanProcessor.cs Outdated Show resolved Hide resolved
@jamescrosswell jamescrosswell marked this pull request as ready for review September 18, 2023 02:46
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@bitsandfoxes bitsandfoxes left a comment

Choose a reason for hiding this comment

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

Fantastic!

Co-authored-by: Stefan Jandl <reg@bitfox.at>
@jamescrosswell jamescrosswell merged commit bbe4fa9 into main Sep 18, 2023
6 of 7 checks passed
@jamescrosswell jamescrosswell deleted the fix/opentelemetry-users-2616 branch September 18, 2023 22:32
@github-actions
Copy link
Contributor

Fails
🚫 Please consider adding a changelog entry for the next release.

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

- Identifying Users with OpenTelemetry doesn't work ([#2618](https://github.com/getsentry/sentry-dotnet/pull/2618))

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 b833430

@yudielcurbelo
Copy link

ISentryUserFactory is an internal interface, so how are we supposed to use it?

internal interface ISentryUserFactory
{
    public User? Create();
}
public class UserFactory : ISentryUserFactory
{
    public User Create()
    {
        ...
    }
}

image

@jamescrosswell
Copy link
Collaborator Author

ISentryUserFactory is an internal interface, so how are we supposed to use it?

Hey @yudielcurbelo, ISentryUserFactory is just an implementation detail that let's the SentrySdk access this information when using OpenTelemetry. Under the hood, all it does is let us get at the HttpContext.User:

var principal = context.User;

So you shouldn't need to use this directly. You'd use whatever Authorization scheme you're using already and Sentry will be able to pull details for the user from the HttpContext.

@ErlendSB
Copy link

How should we rewrite the example stated here: https://docs.sentry.io/platforms/dotnet/guides/aspnetcore/#capturing-the-affected-user

public void ConfigureServices(IServiceCollection services) { services.AddSingleton<IUserFactory, MyUserFactory>(); }

@jamescrosswell
Copy link
Collaborator Author

How should we rewrite the example stated here: https://docs.sentry.io/platforms/dotnet/guides/aspnetcore/#capturing-the-affected-user

public void ConfigureServices(IServiceCollection services) { services.AddSingleton<IUserFactory, MyUserFactory>(); }

Ah, I see. You want to create your own User Factory that gets used by the OpenTelemetry integration? In that case you're right... we'd need to makeISentryUserFactory public.

Thanks for calling this out.

I've created a new ticket to track this: #2714

@@ -0,0 +1,6 @@
namespace Sentry;

internal interface ISentryUserFactory
Copy link

Choose a reason for hiding this comment

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

@jamescrosswell I'm hitting the IUserFactory warning in our builds but when I went to do the cutover to ISentryUserFactory to silence them I discovered this interface is marked internal so I can't implement it in my projects. Was this unintentional? If intentional, how are we supposed to proceed here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey @jeremy-allocate , see #2618 (comment) above.

If it's an emergency and you just need to get a build out, you can disable the warning with a pragma, as we've done here:

#pragma warning disable CS0618
services.TryAddSingleton<IUserFactory, DefaultUserFactory>();
#pragma warning restore CS0618

Choose a reason for hiding this comment

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

All good, thanks @jamescrosswell. We'll live with the warnings until #2714 is released.

@jamescrosswell
Copy link
Collaborator Author

As an FYI, we just made Release 3.40.1 that addresses this:

@jeremy-allocate
Copy link

Awesome, many thanks!

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.

Identifying Users with OpenTelemetry doesn't work
6 participants