feat: read db operation name from telemetry_options #368
Conversation
| defp maybe_add_db_operation(attributes, nil) do | ||
| attributes | ||
| end | ||
|
|
||
| defp maybe_add_db_operation(attributes, db_operation) do | ||
| Map.put(attributes, Trace.db_operation(), db_operation) | ||
| end |
There was a problem hiding this comment.
In general, should I care about guarding against nil, or is that something that downstream would filter out? Or should I even bother sending a nil value in case it is nil?
I see nil values in my traces, so I want to confirm the general rule around the topic.
| @@ -82,7 +83,7 @@ defmodule OpentelemetryEcto do | |||
| def handle_event( | |||
| event, | |||
| measurements, | |||
| %{query: query, source: source, result: query_result, repo: repo, type: type}, | |||
| %{query: query, source: source, result: query_result, repo: repo, type: type, options: options}, | |||
There was a problem hiding this comment.
This presumes options is available. Is that accurate?
There was a problem hiding this comment.
Based on the docs: https://github.com/elixir-ecto/ecto/blob/cba494787b8e98aa0038f6c35e2f44d3661f2620/lib/ecto/repo.ex#L169-L170 it sounds that it should always be there.
But, it seems to be introduced in v3.4.0 https://github.com/elixir-ecto/ecto/blob/cba494787b8e98aa0038f6c35e2f44d3661f2620/CHANGELOG.md?plain=1#L606
| end | ||
|
|
||
| defp maybe_add_db_operation(attributes, db_operation) do | ||
| Map.put(attributes, Trace.db_operation(), db_operation) |
There was a problem hiding this comment.
This is not a valid attribute any longer so this would need to be introduced with semconv 1.26. It's also experimental so it'll have to be opt-in.
For db.operation.name the value is supposed to be the command or operation being invoked (function name, SQL operation, etc), so I'm not sure it can be changed by the user.
[5]: It is RECOMMENDED to capture the value as provided by the application without attempting to do any case normalization. If the operation name is parsed from the query text, it SHOULD be the first operation name found in the query. For batch operations, if the individual operations are known to have the same operation name then that operation name SHOULD be used prepended by BATCH , otherwise db.operation.name SHOULD be BATCH or some other database system specific term if more applicable.
There was a problem hiding this comment.
Should I hard-code it for now without using Trace.*? I intended to use SemConv. I wasn't sure what to do about it.
For db.operation.name the value is supposed to be the command or operation being invoked (function name, SQL operation, etc), so I'm not sure it can be changed by the user.
I asked for direction in the Slack channel, the wording and examples are confusing me!
There was a problem hiding this comment.
@yordis you can look at opentelemetry_xandra where I use all hardcoded semcov names exactly because we are waiting for 1.26
There was a problem hiding this comment.
Either way it is an experimental attribute which requires part of the rewrite of accepted options and overall processing. I'm happy to work through what the values can be so that it can go in there but it will not be published in a release before then.
There was a problem hiding this comment.
That works for me!
Based on the description, it could be anything from the app, so I think my intent is correct.
Maybe we should follow up with OTEL folks; regardless, I would love to have a mechanism to have an identity for the operation.
Maybe I should expand the feature to allow passing of any value into the span telemetry_options: [otel_attributes: [...]] (to make a point)
yordis
force-pushed
the
add-operation-name
branch
from
1d9bc8f
to
51ce033
Compare
|
@bryannaegele, given that there is some key under the SemConv (do not get cut up on the one I used), should we wait until the SemConv changes are done, or could we manually add it and add the function later? |
|
After reading https://opentelemetry.io/docs/specs/semconv/database/sql/ I am even more confused; The intent is to add the identity of a given call to the database so that I can identify where the call may be coming from.
The query is dynamic, and it doesn't have a limit of characters, so systems like CloudWatch could not ingest the data as is. Other things like table name and operation name may tell me what happened, but not where it came from, so I can not observe the metric where the optimizations should be performed without hunting for where the query may be coming from. So, I am not sure what the answer is here. In the worst case, maybe allow a key to be passed in as "additional attributes."?! |
|
From https://opentelemetry.io/docs/specs/semconv/database/database-metrics/
Wouldn't that imply that passing is OK, as I am doing it in the PR? The second part is about "If the operation name is parsed from the query text" |
yordis
force-pushed
the
add-operation-name
branch
from
51ce033
to
307be3a
Compare
Wouldn't the parent span identify the caller? Trying to understand what you're wanting to achieve just because I believe this field is only for the precise name of the operation being executed rather than the context of the caller. I'm guessing that is the case?
That line just says we shouldn't modify what is parsed, e.g. And just so we're on the same page, this library only does spans at this time, so this section is the relevant one. Let's see what the WG has to say. A complicating factor here is this value is conditionally part of the span name so any solution has to take that into account for all use cases. |
|
The tricky bit here, we are using exclusively |
Context
Ecto allows to pass
:telemetry_optionsas a shared options https://github.com/elixir-ecto/ecto/blob/cba494787b8e98aa0038f6c35e2f44d3661f2620/lib/ecto/repo.ex#L93-L94I use that to set the
db.operationOTEL value.