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: add event tracing #3546

Merged
merged 1 commit into from Jun 19, 2023
Merged

feat: add event tracing #3546

merged 1 commit into from Jun 19, 2023

Conversation

hperl
Copy link
Contributor

@hperl hperl commented Jun 15, 2023

This implements the following events from https://github.com/ory-corp/cloud/issues/4028:

OAuth2 & OIDC

  • OIDC initiated login succeeded
  • OIDC initiated login failed
  • Consent granted & revoked
  • Consent grant aborted

Machine activity: Tokens, Permission checks
OAuth2

  • Access tokens created
  • Access token not created
  • Access token inspected
  • Access token check with result

Related issue(s)

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

@hperl hperl requested a review from aeneasr as a code owner June 15, 2023 10:38
@hperl hperl requested a review from kmherrmann June 15, 2023 10:38
@hperl hperl self-assigned this Jun 15, 2023
kmherrmann
kmherrmann previously approved these changes Jun 15, 2023
Copy link
Contributor

@kmherrmann kmherrmann left a 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
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #3546 (6fabd49) into master (b183040) will decrease coverage by 0.02%.
The diff coverage is 74.50%.

❗ Current head 6fabd49 differs from pull request most recent head b960002. Consider uploading reports for the commit b960002 to get more accurate results

@@            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     
Impacted Files Coverage Δ
driver/registry.go 23.52% <ø> (ø)
driver/factory.go 53.70% <14.28%> (-5.48%) ⬇️
driver/registry_base.go 83.69% <16.66%> (-1.55%) ⬇️
oauth2/handler.go 67.13% <83.33%> (+0.39%) ⬆️
consent/handler.go 64.16% <100.00%> (+0.60%) ⬆️
persistence/sql/persister_oauth2.go 84.28% <100.00%> (-2.13%) ⬇️
x/events/events.go 100.00% <100.00%> (ø)

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
Copy link
Contributor

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?

Copy link
Contributor Author

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 ?

Copy link
Member

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, fixed!

Copy link
Contributor

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.

Copy link
Member

@aeneasr aeneasr Jun 19, 2023

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!

oauth2/handler.go Outdated Show resolved Hide resolved
Copy link
Member

@aeneasr aeneasr left a 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

@aeneasr
Copy link
Member

aeneasr commented Jun 19, 2023

@hperl
Copy link
Contributor Author

hperl commented Jun 19, 2023

@aeneasr aeneasr merged commit 44ed0ac into master Jun 19, 2023
28 checks passed
@aeneasr aeneasr deleted the hperl/add-oauth-events branch June 19, 2023 11:22
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.

None yet

4 participants