[pkg/ottl] Accept string (32 byte hex) {span,trace,profile} IDs#43882
Conversation
|
|
|
Welcome, contributor! Thank you for your contribution to opentelemetry-collector-contrib. Important reminders:
A maintainer will review your pull request soon. Thank you for helping make OpenTelemetry better! |
|
I've added a fix for the linter issues, but it looks like perhaps an approval is required to run CI again? |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
please take a look at the conflict and mark ready to review again. |
edmocosta
left a comment
There was a problem hiding this comment.
Thank you @andrewvc and I'm sorry for the long delay to review.
To keep it consistent, we also need to change the SpanID and ProfileID functions. If you agree, I'd be fine reviewing/including them on this same PR (changes are small enough), otherwise we can open separate PRs for that. Thanks!
|
Benchmarks against main here: https://gist.github.com/andrewvc/4b9c73446cc1f9592177d3b980864c3a |
| b.Run("literal_bytes_get_and_set", func(b *testing.B) { | ||
| literalBytes := []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16} | ||
| literalGetter := makeLiteralIDGetter(literalBytes) | ||
| literalGetter, _ := ottl.NewTestingLiteralGetter(true, makeIDGetter(literalBytes)) |
There was a problem hiding this comment.
This is a benchmark, no need to handle the possible error here IMHO
There was a problem hiding this comment.
Adding require.NoError(b, err) would do the work and make any possible failure clearer than panicking while executing the function. It's ok for this specific one, but considering how easy it's for handling that, I wouldn't ignore it :)
There was a problem hiding this comment.
It's not a huge deal for me, so I've added it. I don't love adding unneeded code in benchmarks, but in this case it makes no difference
| b.Run("literal_bytes_get_and_set", func(b *testing.B) { | ||
| literalBytes := []byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16} | ||
| literalGetter := makeLiteralIDGetter(literalBytes) | ||
| literalGetter, _ := ottl.NewTestingLiteralGetter(true, makeIDGetter(literalBytes)) |
There was a problem hiding this comment.
Adding require.NoError(b, err) would do the work and make any possible failure clearer than panicking while executing the function. It's ok for this specific one, but considering how easy it's for handling that, I wouldn't ignore it :)
|
Thank you for your contribution @andrewvc! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. If you are getting started contributing, you can also join the CNCF Slack channel #opentelemetry-new-contributors to ask for guidance and get help. |
Description
Currently
{TraceID,SpanID,ProfileID}only support byte IDs, however in #43429 it was brought up that it is difficult to take a string representation of an ID and directly use that to set a trace ID. This would be for a hexadecimal string representation comprising 32 bytes. This change allows the{TraceID,SpanID,ProfileID}functions to also work on string inputs.In short, the following now works:
Link to tracking issue
Fixes #43429
Testing
In addition to the included go tests the following config
test-str-trace.yml
was used to manually test by invoking
make otelcontribcol && ./bin/otelcontribcol_darwin_arm64 --config test-str-trace.ymlin one window andtelemetrygen traces --otlp-insecure --traces 1in another. The output is as shown:CLI Output
Documentation
I don't currently see documentation for these functions, but I'm new here, maybe I'm missing something? Glad to update it