feat(v2): add TransportTelemetryData for dynamic transport attributes#481
Conversation
westarle
force-pushed
the
westarle/metrics-transport-data-independent
branch
from
d6f21a1 to
d80099f
Compare
|
|
||
| // ExtractTransportTelemetry retrieves a mutable TransportTelemetryData pointer from the context. | ||
| func ExtractTransportTelemetry(ctx context.Context) (*TransportTelemetryData, bool) { | ||
| data, ok := ctx.Value(transportTelemetryKey{}).(*TransportTelemetryData) |
There was a problem hiding this comment.
why not just return ctx.Value() without the variables? You're not doing anything with the (data,ok) values.
There was a problem hiding this comment.
Because this is a type assertion, if I "return" it it only provides the single data value.
I was thinking an 'ok' on the API is idiomatic (vs nil-checking data). WDYT?
There was a problem hiding this comment.
Ahh yes, I forgot about the optionality of the bool for a type assertion, that's on me.
You could also consider a single return and document that it can return nil if there was no data? The user isn't directly providing the input to the extractor where they're going to do anything different if something goes wrong.
e.g.
if data := ExtractTransportTelemetry(ctx); data != nil {
// use the data
}
The other route is if we think extraction could become more complex over time is to use an error type for the second return value, but I don't think we're moving in that direction?
There was a problem hiding this comment.
consider a single return and document that it can return nil if there was no data
I think this is the best thing.
extraction could become more complex over time
Hope not!
shollyman
left a comment
shollyman
left a comment
There was a problem hiding this comment.
One parting thought: you've marked the struct experimental, but not the functions. We should probably add an experiment disclaimer on those as well if you want a little more leeway to evolve.
2 participants
No description provided.