-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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: add event tracing #3546
feat: add event tracing #3546
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.
Content-wise looks great, although i can't vouch for all code changes :)
Codecov Report
@@ Coverage Diff @@
## master #3546 +/- ##
==========================================
- Coverage 76.63% 76.62% -0.02%
==========================================
Files 128 129 +1
Lines 9515 9630 +115
==========================================
+ Hits 7292 7379 +87
- Misses 1730 1749 +19
- Partials 493 502 +9
|
x/events/events.go
Outdated
AccessTokenCreated semconv.Event = "OAuth2AccessTokenCreated" //nolint:gosec | ||
|
||
// AccessTokenNotCreated will be emitted by requests to POST /oauth2/token in case the request was unsuccessful. | ||
AccessTokenNotCreated semconv.Event = "OAuth2AccessTokenNotCreated" //nolint:gosec |
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.
This feels weird. Do we really need this?
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.
It was in the list of needed events, so I guess? @kmherrmann ?
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.
Maybe make it more generic and call it OAuh2FlowError ?
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.
Sounds good, fixed!
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.
We probably don't need this event at all. What it's trying to signal is some error during the OAuth2 flow. But many many things can go wrong during the flow, and we're not about to signal all of them to our analytics platform, are we?
Errors typically don't get emitted as analytics events but go to the monitoring platform (Grafana) as logs or error traces.
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.
Good points Arne! I think we can still ship this for now and invest some time in better observability next. We don't have to use the events and if they serve no purpose in Analytics, we'll remove them. I agree though that this is a different concern from analytics!
494245f
to
0993468
Compare
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.
Very nice, the naming of tokennotcreated is weird though
0993468
to
ec7144a
Compare
ec7144a
to
b960002
Compare
This implements the following events from https://github.com/ory-corp/cloud/issues/4028:
OAuth2 & OIDC
Machine activity: Tokens, Permission checks
OAuth2
Related issue(s)
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments