Inject span #55
Inject span #55
Conversation
…ors. The parent span ID was being formatted incorrectly, which meant all spans except the root span were rejected by the GCP tracing API. If clients did not have error logging hooked up to WithOnError, this would cause child spans to be silently dropped. The documentation says the parent span ID should be formatted as a 64-bit integer encoded as a string, but appears to accept the hexadecimal format returned by SpanID.String().
Fix 'rpc error: code = InvalidArgument desc = Invalid parent id!' err…
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
| @@ -206,11 +212,30 @@ func newContextWithTimeout(ctx context.Context, timeout time.Duration) (context. | |||
| return context.WithTimeout(ctx, timeout) | |||
| } | |||
|
|
|||
| // SetResLabelMapForOnce intializes ResLabelMap for once since | |||
| // monitored resources are static | |||
| func (e * Exporter) SetResLabelMapForOnce(input map[string]string) { | |||
There was a problem hiding this comment.
Even though it's true that monitored resources are static, OT doesn't preclude the possibility of specifying different resource info for different telemetry data.
For example, see this proposal which would allow different resource scopes: open-telemetry/oteps#78
As such, we shouldn't make the assumption of static resources in the exporter. Instead, you should just translate from the Otel resource in the trace data to the expected attribute values, similar to what you did for the metrics resource mapping code in the other PR.
Having said that, this does come at the cost of performance. We may end up looking to optimize this somewhat in the future, but I don't think we should do that now.
There was a problem hiding this comment.
Hmm I should have read your other PR first sorry as that makes it more clear what you're trying to achieve here.
Getting the resource from the controller config might actually be a reasonable thing to do (perhaps it would be better to get it from the top-level tracer/meter config). This is a somewhat dependent on how the SDK changes over time, but we could probably fix this if the SDK changed significantly.
Interesting idea - let me think about it some more and get back to you tomorrow.
FYI @nilebox
There was a problem hiding this comment.
No problem. In fact, the PR(https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/53/files) needs to be changed because I plan to export labels from the merged exporter instead of from cloudmonitoring.go or metric.go. But I may still use Config to extract resources from push.Option. More details are in my design doc.
There was a problem hiding this comment.
👍 - commented on design doc
This includes functions of injecting labels into trace spans and initializing label maps by one time