-
Notifications
You must be signed in to change notification settings - Fork 183
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
Update database span name: clarify target
and remove fallback
#1069
Update database span name: clarify target
and remove fallback
#1069
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.
Nice 👍
I'd maybe add 2 things here:
- The example in the PR description with the anonymous table is very useful, I think that could be part of the spec.
- The goal here is to avoid falling back to a less useful
target
, (that's also part of the PR description here). So my understanding is, we are ok to omittarget
in edge cases. But it's not really written into the spec and this PR doesn't really says that explicitly. I think it'd be worth adding it as well.
I had this in #1061:
A given instrumentation SHOULD strive for consistency in span names.
If certain queries lack this specific attribute, the instrumentation SHOULD use `{db.operation.name}` as the span name and omit `{target}`,
rather than falling back to the next attribute in the [`{target}`](#target-placeholder) list.
(dropped the part which is different in this PR). Do we want to maybe add that part?
Co-authored-by: Trask Stalnaker <[email protected]>
…lable. Add example
82ca546
to
fca4393
Compare
@gregkalapos I updated the doc with your suggestion, lmk what you think |
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.
@gregkalapos I updated the doc with your suggestion, lmk what you think
Thanks, lgtm.
Co-authored-by: Trask Stalnaker <[email protected]>
Fixes #1045
Alternative to #1061
Changes
Clarifies that target depends on the operation and if target is not available, instrumentation should not fallback to other options.
E.g. for query like
SELECT * FROM (VALUES (1, 'a'), (2, 'b'), (3, 'c')) AS t (id, name)
that uses anonymous table, the span name should beSELECT
and notSELECT {db.namespace}
.Merge requirement checklist
[chore]