WIP: Datadog Baggage API #3043
WIP: Datadog Baggage API #3043
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 5121 Passed, 70 Skipped, 2m 33.69s Total Time |
ddtrace/ddtrace.go
Outdated
| @@ -72,13 +72,23 @@ type Span interface { | |||
| // a representative name for a group of spans (e.g. "grpc.server" or "http.request"). | |||
| SetOperationName(operationName string) | |||
|
|
|||
| // BaggageItem returns the baggage item held by the given key. | |||
| BaggageItem(key string) string | |||
There was a problem hiding this comment.
Is this a breaking change to an existing API?
There was a problem hiding this comment.
yes, I think so... I think I should revert this change. But I also think it's slightly counterintuitive to call getBaggage baggageItem. Regardless, I think sticking to the original is better. And, in the future, we could possibly deprecate this name and change it to getBaggageItem depending on what people think
There was a problem hiding this comment.
In terms of breaking changes, yeah reverting this for now is probably a good idea.
But regarding language convention, I would ask the Go guild for guidance. I don't know if this is common in Go. Just to give you a different example, in .NET we are exposing baggage as an IDictionary<string, string>, so the API doesn't use functions and there is no get or set in the name, but it's functionally equivalent:
// set
Baggage.Current["name"] = "value";
// get
var value = Baggage.Current["name"];(that's unrelated to the fact that our baggage is separate from spans)
There was a problem hiding this comment.
As Lucas said, this change would break backwards compatibility. A few notes:
- The existing BaggageItem and SetBaggageItem methods already handle the single baggage item operations described in the RFC. You can make small changes within these methods in the span.go file if necessary. However, if the changes will break backward compatibility, we can instead introduce new methods to the interface to meet the RFC requirements.
- In Go, it's conventional to avoid using "Get" in function names for getters. So, instead of naming the function GetBaggageItem, we would simply call it BaggageItem.
There was a problem hiding this comment.
The existing BaggageItem and SetBaggageItem have not been changed. I've only added GetAllBaggageItems, RemoveBaggage and RemoveAllBaggageItems. There's no need to edit or add to the existing functions as they function according to the RFC. I'm not sure about the naming for the new functions. Those can be discussed in the guild meeting!
There was a problem hiding this comment.
PR Name Change
It's my understanding that the Baggage RFC has 3 components: (1) Implement Datadog API for baggage, (2) Support baggage propagation via http headers, (3) Support Baggage API for opentelemetry drop-in support.
Because both 1 and 3 refer to "baggage APIs," let's make the scope of this PR clearer by renaming it to something that specifies "Datadog API."
Implementation approach
The general approach looks good, but let's run this by the go guild members before proceeding.
ddtrace/ddtrace.go
Outdated
| @@ -72,13 +72,23 @@ type Span interface { | |||
| // a representative name for a group of spans (e.g. "grpc.server" or "http.request"). | |||
| SetOperationName(operationName string) | |||
|
|
|||
| // BaggageItem returns the baggage item held by the given key. | |||
| BaggageItem(key string) string | |||
There was a problem hiding this comment.
As Lucas said, this change would break backwards compatibility. A few notes:
- The existing BaggageItem and SetBaggageItem methods already handle the single baggage item operations described in the RFC. You can make small changes within these methods in the span.go file if necessary. However, if the changes will break backward compatibility, we can instead introduce new methods to the interface to meet the RFC requirements.
- In Go, it's conventional to avoid using "Get" in function names for getters. So, instead of naming the function GetBaggageItem, we would simply call it BaggageItem.
What does this PR do?
Adding baggage API. This includes
getAllBaggageItems,removeBaggageItem, andremoveAllBaggageItems.baggageItemhas been renamed togetBaggageItemto align with the Baggage RFC as well as other tracers that have already implemented baggage. This is the first PR to add baggage functionality to dd-trace-go and only adds the public API methods.Motivation
Reviewer's Checklist
v2-devbranch and reviewed by @DataDog/apm-go.Unsure? Have a question? Request a review!