Closed mariusc83 closed 4 months ago
Attention: Patch coverage is 90.47619%
with 2 lines
in your changes are missing coverage. Please review.
Project coverage is 63.92%. Comparing base (
478ba84
) to head (a0e5b5f
). Report is 1 commits behind head on feature/otel-support.
Important
It feels like we need to review the way we send the configuration telemetry information for other features than RUM. Right now we detect the OT by reflection, and this PR tries to add the OpenTelemetry version in the RUM build config. This doesn't make any sense and adds noise to a feature that should not have those kind of information. Maybe we should consider a different way for features to communicate this information and RUM would be agnostic of the keys and values.
@xgouchet I understand your concern, we could send it in the Trace
feature context as we do for the is_opentelemetry_enabled
property. But we still need to keep the build config entry as the other option is to send it as a hardcoded value that we will definitely forget to update. What do you think ?
P.S. I think opening a RFC only for this topic only would be too much IMO. @plousada what do you think ?
@xgouchet I understand your concern, we could send it in the
Trace
feature context as we do for theis_opentelemetry_enabled
property. But we still need to keep the build config entry as the other option is to send it as a hardcoded value that we will definitely forget to update. What do you think ?P.S. I think opening a RFC only for this topic only would be too much IMO. @plousada what do you think ?
I don't have an issue having the info set by build config, it just would make more sense to do it on the otel/tracing module, instead of the RUM one
@xgouchet I understand your concern, we could send it in the
Trace
feature context as we do for theis_opentelemetry_enabled
property. But we still need to keep the build config entry as the other option is to send it as a hardcoded value that we will definitely forget to update. What do you think ? P.S. I think opening a RFC only for this topic only would be too much IMO. @plousada what do you think ?I don't have an issue having the info set by build config, it just would make more sense to do it on the otel/tracing module, instead of the RUM one
@xgouchet I understand your concern, we could send it in the
Trace
feature context as we do for theis_opentelemetry_enabled
property. But we still need to keep the build config entry as the other option is to send it as a hardcoded value that we will definitely forget to update. What do you think ? P.S. I think opening a RFC only for this topic only would be too much IMO. @plousada what do you think ?I don't have an issue having the info set by build config, it just would make more sense to do it on the otel/tracing module, instead of the RUM one
Yes this is what I am changing now, makes more sense to have it there. I am pushing a commit in few seconds.
What does this PR do?
Following the iOS Pr we are going to report the usage of the
OpenTelemetry
API in our configuration telemetry as follows:hasTrace:true | false
,tracerApi: OpenTracing | OpenTelemetry | null
,traceApiVersion: [openTelemetryApiVersion] | null
Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)