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

Delete a baggage item when value is blank #562

Merged
merged 1 commit into from Mar 29, 2021

Conversation

zacscoding
Copy link
Contributor

Which problem is this PR solving?

  • delete a baggage item in span if provided empty value.

Short description of the changes

  • if sets to blank values in span context, then delete a baggage item.

@codecov
Copy link

codecov bot commented Mar 15, 2021

Codecov Report

Merging #562 (7c867de) into master (fe3fa55) will increase coverage by 1.89%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #562      +/-   ##
==========================================
+ Coverage   88.64%   90.53%   +1.89%     
==========================================
  Files          61       24      -37     
  Lines        3311     1670    -1641     
==========================================
- Hits         2935     1512    -1423     
+ Misses        249      109     -140     
+ Partials      127       49      -78     
Impacted Files Coverage Δ
span_context.go 94.30% <100.00%> (+0.34%) ⬆️
testutils/udp_transport.go
internal/baggage/remote/restriction_manager.go
testutils/sampling_manager.go
rpcmetrics/metrics.go
log/zap/logger.go
utils/reconnecting_udp_conn.go
internal/baggage/remote/options.go
utils/udp_client.go
transport/http.go
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe3fa55...7c867de. Read the comment docs.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

please make sure all commits are signed (see CONTRIBUTING)

span_context.go Outdated
Comment on lines 330 to 338
if _, ok := c.baggage[key]; ok {
newBaggage = make(map[string]string, len(c.baggage))
for k, v := range c.baggage {
newBaggage[k] = v
}
delete(newBaggage, key)
} else {
newBaggage = c.baggage
}
return SpanContext{c.traceID, c.spanID, c.parentID, newBaggage, "", c.samplingState, c.remote}
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
if _, ok := c.baggage[key]; ok {
newBaggage = make(map[string]string, len(c.baggage))
for k, v := range c.baggage {
newBaggage[k] = v
}
delete(newBaggage, key)
} else {
newBaggage = c.baggage
}
return SpanContext{c.traceID, c.spanID, c.parentID, newBaggage, "", c.samplingState, c.remote}
if _, ok := c.baggage[key]; !ok {
return c
}
newBaggage = make(map[string]string, len(c.baggage))
for k, v := range c.baggage {
newBaggage[k] = v
}
delete(newBaggage, key)
return SpanContext{c.traceID, c.spanID, c.parentID, newBaggage, "", c.samplingState, c.remote}

ctx = ctx.WithBaggageItem("some-KEY", "")
assert.Nil(t, ctx.baggage)
ctx = ctx.WithBaggageItem("some-KEY", "Some-Value")
assert.Equal(t, map[string]string{"some-KEY": "Some-Value"}, ctx.baggage)
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
assert.Equal(t, map[string]string{"some-KEY": "Some-Value"}, ctx.baggage)
assert.Equal(t, map[string]string{"some-KEY": "Some-Value"}, ctx.baggage)
ctx = ctx.WithBaggageItem("another-KEY", "")
assert.Equal(t, map[string]string{"some-KEY": "Some-Value"}, ctx.baggage)

Comment on lines 88 to 89
ctx = ctx.WithBaggageItem("some-KEY", "")
assert.Equal(t, map[string]string{}, ctx.baggage)
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
ctx = ctx.WithBaggageItem("some-KEY", "")
assert.Equal(t, map[string]string{}, ctx.baggage)
ctx2 := ctx.WithBaggageItem("some-KEY", "")
assert.Equal(t, map[string]string{"some-KEY": "Some-Value"}, ctx.baggage, "parent unchanged")
assert.Equal(t, map[string]string{}, ctx2.baggage)

Signed-off-by: zacscoding <zaccoding725@gmail.com>
@zacscoding
Copy link
Contributor Author

@yurishkuro thx for review :) updated all comments 👍

@yurishkuro yurishkuro changed the title Add delete a baggage item in span Delete a baggage item when value is blank Mar 29, 2021
@yurishkuro yurishkuro merged commit 2005270 into jaegertracing:master Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants