Skip to content

fix(n1-api-calls): Omit span URL subdomain from fingerprinting #43706

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

Merged

Conversation

gggritso
Copy link
Member

Closes PERF-1937. Sometimes we get spans with URLs like "http://<client>.service.io/resource" in them. The <client> is a hard-to-parameterize string of some kind. This causes a fingerprint explosion if different transactions are getting data from different clients.

Omit the subdomain from fingerprinting (but leave detection alone). For fingerprinting, we actually want to ignore this, and only fingerprint on the path. There's basically no scenario where an application would use the same path structure from different domains and it's not an error.

When fingerprinting spans, ignore the subdomain. It's not useful for
fingerprinting, and causes incorrect fingerprint changes.
Copy link
Contributor

@DominikB2014 DominikB2014 left a comment

Choose a reason for hiding this comment

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

A test case would probably be good to add here.

@gggritso
Copy link
Member Author

Yeah I should write some fingerprinting tests, I just haven't figured out a good way to do it yet. For N+1 API we need 10 or more spans, and they're hard to construct 🤔 I have a Jira task to increase coverage, though, so I'll do that soon!

Copy link
Member

@k-fish k-fish left a comment

Choose a reason for hiding this comment

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

@gggritso I think the fingerprinting probably needs the most coverage since it has some permutations, doesn't create_span from event_generators do the trick for making more spans? (using the SpanBuilder?)

@DominikB2014
Copy link
Contributor

DominikB2014 commented Jan 26, 2023

Yeah I should write some fingerprinting tests, I just haven't figured out a good way to do it yet. For N+1 API we need 10 or more spans, and they're hard to construct 🤔 I have a Jira task to increase coverage, though, so I'll do that soon!

Hmm, not sure how to solve the spans being hard to construct, i've just been using create_span like @k-fish mentioned. But for fingerprint tests in general, I found it helpful not to test a single fingerprint, but to test that things get grouped correctly (aka 2 or more events have matching fingerprints),

@gggritso
Copy link
Member Author

Added test coverage in #43732 for the sake of accuracy

@gggritso
Copy link
Member Author

@DominikB2014 @k-fish merged my test coverage PR, so now this PR includes a test 👍🏻

Copy link
Contributor

@DominikB2014 DominikB2014 left a comment

Choose a reason for hiding this comment

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

lgtm!

@gggritso gggritso merged commit 4cce775 into master Jan 30, 2023
@gggritso gggritso deleted the fix/PERF-1937-omit-subdomain-from-n1-api-call-fingerprint branch January 30, 2023 15:07
mikejihbe pushed a commit that referenced this pull request Feb 6, 2023
Closes PERF-1937. Sometimes we get spans with URLs like
`"http://<client>.service.io/resource"` in them. The `<client>` is a
hard-to-parameterize string of some kind. This causes a fingerprint
explosion if different transactions are getting data from different
clients.

Omit the subdomain from fingerprinting (but leave detection alone). For
fingerprinting, we actually _want_ to ignore this, and only fingerprint
on the path. There's basically no scenario where an application would
use the same path structure from different domains and it's _not_ an
error.
@github-actions github-actions bot locked and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants