Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BytesMapCarrier #2245

Closed
wants to merge 6 commits into from
Closed

Add BytesMapCarrier #2245

wants to merge 6 commits into from

Conversation

rakyll
Copy link
Contributor

@rakyll rakyll commented Sep 16, 2021

BytesMapCarrier makes it easy for users to inject/extract propagated values from various sources. For example, if a trace context is propagated by an environment variable, users can create a new BytesMapCarrier with values from the environment and inject the trace context into a Go context.

Fixes #2243.

@codecov
Copy link

codecov bot commented Sep 16, 2021

Codecov Report

Merging #2245 (878c1c7) into main (c577314) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2245   +/-   ##
=====================================
  Coverage   73.6%   73.6%           
=====================================
  Files        175     175           
  Lines      12409   12419   +10     
=====================================
+ Hits        9134    9147   +13     
+ Misses      3041    3039    -2     
+ Partials     234     233    -1     
Impacted Files Coverage Δ
propagation/propagation.go 85.7% <100.0%> (+4.4%) ⬆️
...s/otlp/otlptrace/internal/connection/connection.go 16.2% <0.0%> (+1.5%) ⬆️

@Aneurysm9 Aneurysm9 added area:propagators Part of OpenTelemetry context propagation enhancement New feature or request release:after-1.0 labels Sep 16, 2021
BytesMapCarrier makes it easy for users to inject/extract propagated values from various sources. For example, if a trace context is propagated by an environment variable, users can create a new BytesMapCarrier with values from the environment and inject the trace context into a Go context.
@pellared
Copy link
Member

Would it not be better if this propagator lives in OTel Go Contrib as it is not required not defined by the spec?

@Aneurysm9
Copy link
Member

Would it not be better if this propagator lives in OTel Go Contrib as it is not required not defined by the spec?

I don't think the HeaderCarrier we provide here is required by the spec either. I think this is worth having in the main propagation package rather than requiring users to import another module just for this broadly useful carrier.

Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks good to me. Please fix the flaky test and add a CHANGELOG entry.

}

// Set stores the key-value pair.
func (c BytesMapCarrier) Set(key string, value string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why input the value as a string and store it as a []byte?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmacd So that it implements the TextMapCarrier interface.

@rakyll Maybe it would be worth adding var _ TextMapCarrier = (*BytesMapCarrier)(nil) ?

Copy link
Contributor

@jmacd jmacd Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not store the value as a string, vs. storing as a []byte?

Not blocking, just curiousity. It seems to mean that every call to Get() copies the []byte into a new string because the compiler can't tell that the []byte is never modified.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rakyll would you mind following up, here?

propagation/propagation.go Show resolved Hide resolved
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a little proposal to make it more clear that BytesMapCarrier implements TextMapCarrier interface.

}

// Set stores the key-value pair.
func (c BytesMapCarrier) Set(key string, value string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmacd So that it implements the TextMapCarrier interface.

@rakyll Maybe it would be worth adding var _ TextMapCarrier = (*BytesMapCarrier)(nil) ?

Comment on lines +43 to +44
// BytesMapCarrier is a Carrier that stores propagated
// values in a map of bytes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// BytesMapCarrier is a Carrier that stores propagated
// values in a map of bytes.
// BytesMapCarrier adapts map of bytes to satisfy the TextMapCarrier interface.

@rakyll
Copy link
Contributor Author

rakyll commented Oct 26, 2021

Is it ok for someone to take over this PR? I opened it but have no bandwidth to follow up right now.

@jmacd
Copy link
Contributor

jmacd commented Oct 27, 2021

@rakyll We had asked you a question: #2245 (comment)

@MrAlias
Copy link
Contributor

MrAlias commented Oct 28, 2021

Is it ok for someone to take over this PR? I opened it but have no bandwidth to follow up right now.

I'm happy to take this over, but I think to @jmacd's point we should probably replace this PR with a one adding a type StringMapCarrier map[string]string. I'll start work to open a PR to add that and potentially replace this.

@MrAlias MrAlias mentioned this pull request Oct 28, 2021
@MrAlias
Copy link
Contributor

MrAlias commented Oct 28, 2021

I've created #2334 as a replacement, please take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:propagators Part of OpenTelemetry context propagation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider having a map and request header Carrier in SDK
5 participants