NH-64718: otlp processor to support sampling and response time metrics #101
Conversation
cheempz
changed the title
cheempz
changed the title
cheempz
changed the title
There was a problem hiding this comment.
Another batch of comments @xuan-cao-swi -- let's chat if you'd like to address some of these in follow-on PRs since this one is getting a bit long and afaict the existing solarwinds processor might need some fixing which is a bit out of scope.
Co-authored-by: Lin Lin <lin.lin@solarwinds.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looking pretty good @xuan-cao-swi! One more batch of comments, this should be it once addressed.
| SolarWindsAPM.logger.debug { "[#{self.class}/#{__method__}] processor on_finish span: #{span.to_span_data.inspect}" } | ||
|
|
||
| # return if span is non-entry span | ||
| return if span.parent_span_id != ::OpenTelemetry::Trace::INVALID_SPAN_ID |
There was a problem hiding this comment.
This is incorrect if it returns even for an entry span where the parent is remote. After some digging I think you're not using the non_entry_span method here because on_finish doesn't get passed the parent context, and unfortunately the OTel Ruby SDK for span only makes span.parent_span_id available and not the actual span parent, correct?
One solution is something like our JS custom distro's https://github.com/solarwinds/apm-js/blob/main/packages/sdk/src/parent-info-processor.ts. Another might be to have non_entry_span check for the presence of our proprietary service entry internal attrs attributes which the custom sampler sets... although it may be unsafe to assume our custom sampler will always be used, e.g. tail based sampling. So one other thought is an internal sw.is_entry_span: true attr could be set during on_start for similar purpose.
Whatever the approach, we do need this entry span logic to be correct because otherwise the metrics collected and shown to customers will be very wrong. Let's also chat about how to test for the correct trace metrics, it is a bit hard to automate but I'd like to at least have a session where we go over what the expectations are and how to verify the results in SWO.
There was a problem hiding this comment.
and unfortunately the OTel Ruby SDK for span only makes span.parent_span_id available and not the actual span parent, correct?
Yes, I think that's the reason I didn't use non_entry_span
So one other thought is an internal sw.is_entry_span: true attr could be set during on_start for similar purpose.
I think I will go with this solution. I will come up with some test case for this.
| @exporter&.export([span.to_span_data]) if span.context.trace_flags.sampled? | ||
| return | ||
| end | ||
| return if span.parent_span_id != ::OpenTelemetry::Trace::INVALID_SPAN_ID |
There was a problem hiding this comment.
Same comment as for otlp_processor
Co-authored-by: Lin Lin <lin.lin@solarwinds.com>
Co-authored-by: Lin Lin <lin.lin@solarwinds.com>
Co-authored-by: Lin Lin <lin.lin@solarwinds.com>
Description
Based on
OTLP_METRICSto determine which processor is required for ruby agent.otlp_processortake metrics_exporter to send metrics to otlp procotol collectorTest
With otel extension from swi: trace
Directly send to collector api: trace