Added an option to have baggage through Observation API #688
Conversation
whenever a key-value matches a preconfigured baggage config and an Observation started a scope then automatically baggages will also be started fixes gh-455
|
|
||
| private static List<String> remoteFields(List<String> tagFields, List<String> remoteFields) { | ||
| List<String> combined = new ArrayList<>(tagFields); | ||
| combined.addAll(remoteFields); |
There was a problem hiding this comment.
Why are these combined? It looks like the other baggage managers don't combine them.
There was a problem hiding this comment.
So when you have tag fields it means that it's baggage that additionally will be tagged. Inc case of Brave we never had a constructor that accepts just remote fields and I can't introduce one cause we have a constructor for tag fields already. So it does make sense to combine them. Of course I should combine those for other baggage managers too - I'll look into that, thanks
There was a problem hiding this comment.
Is it better to create a unique set of fields?
The values seem to be used in the handler for contains, so in the end, it doesn't need to be unique here, but just for clarity.
There was a problem hiding this comment.
I think I'm still confused. The assumption seems to be that all tagFields are remote baggage. Is local baggage that should also be tagged not possible? In the test code, it looks like the baggage key to be tagged is local baggage:
.add(BaggagePropagationConfig.SingleBaggageField.local(BaggageField.create(TAG_KEY)))
So shouldn't that not be combined with remote fields?
There was a problem hiding this comment.
From the point of view of a baggage manager there is no concept of local fields. That is really brave specific and has to be configured via propagation configuration. I understand that this might be confusing. We can rename this from remoteFields to baggageFields and that way that will mean all baggage regardless of whether it's local or remote. WDYT?
| */ | ||
| public BraveBaggageManager(List<String> tagFields) { | ||
| public BraveBaggageManager(List<String> tagFields, List<String> remoteFields) { |
There was a problem hiding this comment.
Shouldn't we introduce a new ctor?
Can any of these be null?
There was a problem hiding this comment.
We can't - type inference is the same. They can't be null, we have non null api by default. We can pass empty collections
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except |
There was a problem hiding this comment.
The license format is different for other files.
There was a problem hiding this comment.
That's strange
| then(tracer.getBaggage(KEY_1).get()).isNull(); | ||
| then(tracer.getBaggage(OBSERVATION_BAGGAGE_KEY).get()).isNull(); |
There was a problem hiding this comment.
Can we do these check before opening the scope too?
|
|
||
| private static List<String> remoteFields(List<String> tagFields, List<String> remoteFields) { | ||
| List<String> combined = new ArrayList<>(tagFields); | ||
| combined.addAll(remoteFields); |
There was a problem hiding this comment.
Is it better to create a unique set of fields?
The values seem to be used in the handler for contains, so in the end, it doesn't need to be unique here, but just for clarity.
| } | ||
|
|
||
| private static List<KeyValue> matchingBaggageKeyValues(Tracer tracer, ContextView context) { | ||
| List<String> lowerCaseRemoteFields = new ArrayList<>(); |
There was a problem hiding this comment.
Good to make it a Set for being used with contains for small performance optimization.
There was a problem hiding this comment.
Thanks, I've applied both suggestions
| for (String remoteField : tracer.getBaggageFields()) { | ||
| lowerCaseRemoteFields.add(remoteField.toLowerCase()); | ||
| } | ||
| Set<KeyValue> baggageKeyValues = new HashSet<>(); |
There was a problem hiding this comment.
nit: Different from the lowerCaseRemoteFields variable, this baggageKeyValues variable (and the return type of this method, etc) might be better to keep as List. That way, creating BaggageInScope is always in a predictable order which might give an easier debug experience, just in case. If the ordering doesn't really matter, then Set is ok.
There was a problem hiding this comment.
You're right, I fixed that
|
cc @mhalbritter once we merge this, the constructors for baggagemanagers changed (non breaking change) so in boot we could start using the remote fields property to set the baggage fields on the baggage managers |
whenever a key-value matches a preconfigured baggage config and an Observation started a scope then automatically baggages will also be started
fixes gh-455
TODO:
related #455