Modify ElasticSearch Instrumentation to take advantage of sampling decision #95
Conversation
Codecov Report
@@ Coverage Diff @@
## main #95 +/- ##
=======================================
Coverage 77.51% 77.51%
=======================================
Files 52 52
Lines 1454 1454
=======================================
Hits 1127 1127
Misses 327 327
|
| activity.SetTag(SemanticConventions.AttributeDbName, elasticIndex); | ||
| } | ||
| var elasticIndex = this.GetElasticIndex(uri); | ||
| activity.DisplayName = this.GetDisplayName(activity, method, elasticIndex); |
There was a problem hiding this comment.
DisplayName was already set few lines above. One of them could be removed
There was a problem hiding this comment.
The reason it was set above was because that used to be outside of the check for IsAllDataRequested and gave the activity a basic display name and if IsAllDataRequested was true it set it to a more details display name.
|
@ejsmith Would you be willing to put yourself as a codeowner for this ES instrumentation, and review/approve the PRs? |
Yeah, that would be good with me. |
| return; | ||
| } | ||
|
|
||
| ActivityInstrumentationHelper.SetActivitySourceProperty(activity, ActivitySource); | ||
| ActivityInstrumentationHelper.SetKindProperty(activity, ActivityKind.Client); | ||
| if (activity.IsAllDataRequested) |
There was a problem hiding this comment.
Previously, we were doing some things even when IsAllDataRequested was false. Is it best practice now to not do anything?
There was a problem hiding this comment.
I see that other instrumentations like the HTTP one are still gathering some info even when IsAllDataRequested is false. But this PR is changing it so that we don't do anything. I'm concerned that by removing the basic level of information that the activity spans will be so generic you won't know what they are for.
There was a problem hiding this comment.
If IsAllDataRequested is set to false, we don't export the activity as Processor.OnEnd is not called. So it wouldn't really be an issue if the activity doesn't have that basic level of information as it would never be sent to Processors.
There was a problem hiding this comment.
Hmm... so why does the HTTP instrumentation still do some things then? https://github.com/open-telemetry/opentelemetry-dotnet/blob/caab6d63f47052d9f8fadd40485be313ce171f6e/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs#L85-L110
There was a problem hiding this comment.
HTTP instrumentation does not enrich the activity if IsAllDataRequested is set to false. It only propagates context as we don't want broken traces. If HTTP instrumentation does not propagate context, then the external system which receives the HTTP call will create new TraceId for the same transaction, and this will lead to broken traces. That's why, we ensure that the context is propagated regardless of the sampling decision as the other system that is receiving the HTTP call might upsample the activity and export it. In that case, we want to retain the same TraceId for that operation.
In case of Elasticsearch, there is no context propagation happening. We only enrich the activity, which wouldn't be necessary if we are not going to export that activity.
There was a problem hiding this comment.
Ok, that makes sense. Guess we would just need to de-duplicate the DisplayName setting then.
…ticSearch-Instrumentation-For-SuppressInstrumentation
…on-For-SuppressInstrumentation
Extends the optimization done in PR #1894 to optimize ElasticSearch Instrumentation
Changes
Sdk.SuppressInstrumentationandactivity.IsAllDataRequested