-
Notifications
You must be signed in to change notification settings - Fork 890
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
Add semantic conventions for Elasticsearch client instrumentation #3358
Conversation
specification/trace/semantic_conventions/instrumentation/elasticsearch.md
Outdated
Show resolved
Hide resolved
specification/trace/semantic_conventions/instrumentation/elasticsearch.md
Outdated
Show resolved
Hide resolved
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.
LGTM with the understanding of the upcoming changes #3358 (comment).
@estolfo can you post a comparison of this proposal with the existing Java and Python implementations? I think that would help us pull in and get feedback from Java and Python reviewers. |
PythonThis is what I see from looking at the source code and the tests: Span name:
Special span attributes:
result fields: Other fields
cc @trask |
JavaIt's not so clear to me how the Java instrumentation works. I think they create spans for both the high-level rest client and the transport layer. I think it'd be best to get the java group's input on whether the following is accurate. The span name depends on the level of the instrumentation. From what I can tell, the rest client instrumentation creates a span with the http method as the name, for example Special Span attributes
Other fields
cc @trask |
.NETI'm adding another one for good luck :) Span name: One of the URL template values listed here. Other fields
http attributes / net attributes |
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.
thx!
The elasticsearch url is modified with placeholders in order to reduce the cardinality of the span name. When the url | ||
contains a document id, it SHOULD be replaced by the identifier `{id}`. When the url contains a target data stream or | ||
index, it SHOULD be replaced by `{target}`. | ||
For example, a request to `/test-index/_doc/123` should have the span name `GET /{target}/_doc/{id}`. | ||
When there is no target or document id, the span name will contain the exact url, as in `POST /_search`. |
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.
when looking at a given elasticsearch url, what's the best way for instrumentation to detect which part of it should be replaced by {id}
and which part should be replaced with {target}
? is there a limited set elasticsearch url patterns?
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.
Yes, there is a file in the Elasticsearch specification repo that can be used.
As an example, the .NET client uses this generator to create this path lookup file that is used in the instrumentation.
specification/trace/semantic_conventions/instrumentation/elasticsearch.md
Outdated
Show resolved
Hide resolved
specification/trace/semantic_conventions/instrumentation/elasticsearch.md
Show resolved
Hide resolved
specification/trace/semantic_conventions/instrumentation/elasticsearch.md
Outdated
Show resolved
Hide resolved
| Attribute | Type | Description | Examples | Requirement Level | | ||
|----------------------------|---|----------------------------------------------------------------------|---------------------------------------------------------|------------------------| | ||
| `db.elasticsearch.doc_id` | string | The document that the request targets, specified in the path. | `'123'` | Conditionally Required | | ||
| `db.elasticsearch.target` | string | The name of the data stream or index that is targeted. | `'users'` | Conditionally Required | |
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.
is this also extracted from the path?
| `db.elasticsearch.target` | string | The name of the data stream or index that is targeted. | `'users'` | Conditionally Required | | |
| `db.elasticsearch.target` | string | The name of the data stream or index that is targeted, specified in the path. | `'users'` | Conditionally Required | |
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.
That depends on the implementation of the client. It's possible that the client is written in way such that the target is specified as an argument or part of a hash passed to the function/method. The Ruby client, for example, has the target specified in the hash passed as an arg to the action method.
Example source / usage
So your suggested change makes sense; the target is determined not necessarily from the path, though it can appear as part of the path. I had this wording in there because I wanted to emphasize that the db.elasticsearch.target
span attribute should match exactly what is in the path and what is replaced by {target}
in the span name.
Co-authored-by: Trask Stalnaker <[email protected]>
…icsearch.md Co-authored-by: Trask Stalnaker <[email protected]>
…icsearch.md Co-authored-by: Trask Stalnaker <[email protected]>
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
@trask What are the next steps for this? Have I answered all your questions? |
@open-telemetry/opentelemetry-python-contrib-approvers @open-telemetry/dotnet-contrib-approvers @open-telemetry/java-instrumentation-approvers any thoughts on this, since it affects existing instrumentation in these languages? thx |
@trask We will bring it up at the Python, Java, and .NET SIGs next week if we don't hear from the teams before then. Update: because the .NET instrumentation is native to the Elasitcsearch client itself, I checked with the maintainer of that project and he said that: he can't say with certainty that he can get the information without a reasonable amount of work in the client, but that he suspects it should be possible to add most if not all of these onto the .NET spans in a way that is backwards compatible. |
specification/trace/semantic_conventions/instrumentation/elasticsearch.md
Outdated
Show resolved
Hide resolved
…icsearch.md Co-authored-by: Alexander Wert <[email protected]>
@trask [Not relevant to the discussion in this PR] For what it's worth, they said they'd rather have the instrumentation done natively in the Elasticsearch python client, rather than in the contrib repo. |
@estolfo heads up - most likely this PR will be closed, and we'll ask you to resubmit the PR in a new repo, please refer to #3474 (comment). |
@reyang Sure, no problem, thanks for letting me know. I see that the repo isn't created yet so I'll open a PR when it's available. |
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.
Can you move this to https://github.com/open-telemetry/semantic-conventions?
Opened new PR here |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
tag: call-level-tech-specific | ||
brief: > | ||
The query params of the request, as a json string. | ||
examples: [ '"{\"q\":\"test\"}", "{\"refresh\":true}"' ] |
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.
examples: [ '"{\"q\":\"test\"}", "{\"refresh\":true}"' ] | |
examples: [ 'q=test&refresh=true' ] |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Fixes open-telemetry/semantic-conventions#706
Changes
Add semantic conventions for Elasticsearch client instrumentation span names and attributes.
Related OTEP(s)