-
-
Notifications
You must be signed in to change notification settings - Fork 210
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
Fix: InstallationId now gets evaluated only once per application execution #3529
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wonder about the log level.
return id; | ||
} | ||
// If there's no write permission or the platform doesn't support this, we handle | ||
// and let the next installation id strategy kick in | ||
catch (Exception ex) | ||
{ | ||
_options.LogError(ex, "Failed to resolve persistent installation ID."); | ||
options.LogWarning(ex, "Failed to resolve persistent installation ID."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning for the downgrade in log level? Failing to create a persistent installation ID sounds like a pretty major deal to me.
AFAIK, the whole sessions/release health part relies on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary from this conversation thread (see comments from Christian Klemm):
- When using non-root .NET Docker containers you typically have no write access to the app root directory since you're using a different user.
- I'm not sure whether an unprivileged user has access to network cards
We could revert to logging an error. At least now people would only get one error per application run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep it at error.
The SDK is attempting something that's a prerequesite for a feature and fails to set it up. That's an error. Non-root .NET Docker container users can opt-out of i.e. auto-session-tracking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! That detail in the lazy
!
@@ -77,7 +77,7 @@ internal class InstallationIdHelper(SentryOptions options) | |||
// and let the next installation id strategy kick in | |||
catch (Exception ex) | |||
{ | |||
options.LogWarning(ex, "Failed to resolve persistent installation ID."); | |||
options.LogError(ex, "Failed to resolve persistent installation ID."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do wonder if all three of these log statements need to be LogError
. If we can't store a file with a generated id, it's not a huge problem as long as we can read an machine identifier from the network card.
It feels like this would be an error only if none of the 3 techniques we try to resolve an installation id are successful.
Resolves #3524