Adding check to avoid metrics of telemetry calls #61
Conversation
| count = dependency_map.get("count", 0) | ||
| dependency_map["count"] = count + 1 | ||
| # Only collect request metric if sent from non-exporter thread | ||
| if context.get_value("suppress_instrumentation") is None: |
There was a problem hiding this comment.
Don't we need to set supress_instrumentation in the context for exporter threads?
There was a problem hiding this comment.
We don't need to add it in the exporter, SpanProcessor will add it before calling the exporter
There was a problem hiding this comment.
It seems only SimpleExportSpanProcessor does this? What about the other SpanProcessors? And what about surpressing instrumentation for the metrics exporter as well? Also, are we going to ignore dependency calls that happen during the same time as export?
There was a problem hiding this comment.
BatchExportSpanProcessor also add this, you are totally right this need to be taken care of in OT repo for Metrics, and I'm not sure how we can ensure this applies to all SpanProcessors, maybe a topic for OT SIG meeting
There was a problem hiding this comment.
Created issue in OT to track this
open-telemetry/opentelemetry-python#524
There was a problem hiding this comment.
If OT does not have this, any point in having this PR merged? It also does not address the issue of dependency calls being made at the same time the export is being called.
There was a problem hiding this comment.
Not sure I understand the issue you mention, the point for merging this here is to address the issue for Spans, so we don't send metrics for internal calls to Breeze, the code in Azure repo would not be affected once issue is resolved in OT so once Metrics add correct flag this will be automatically working here. If you think there are big issues with this flag it would be good to bring it up in OT repo because HTTP request ext is heavily relying on this
There was a problem hiding this comment.
What I am referring to is, this mechanism for using suppress_instrumentation does not cover the use case of when a http request call is made DURING exporting (that's not from the exporter itself). With that being said, IF we are still going with the suppress_instrumentation route to address this problem, you are correct that we don't have to change anything in this repo. I would add a comment here indicating that it is contigent on changes in the OT repo.
Codecov Report
@@ Coverage Diff @@
## master #61 +/- ##
==========================================
- Coverage 98.40% 98.27% -0.13%
==========================================
Files 13 13
Lines 751 753 +2
Branches 96 97 +1
==========================================
+ Hits 739 740 +1
Misses 9 9
- Partials 3 4 +1
Continue to review full report at Codecov.
|
lzchen
left a comment
lzchen
left a comment
There was a problem hiding this comment.
Looks good. I would add a comment.
3 participants
Use suppress_instrumentation added in Span processors