Add baggage restriction manager #131
Conversation
| * @param {string} [serviceName] - name of the current service / application, same as given to Tracer | ||
| * @param {object} [options] - optional settings | ||
| * @param {object} [options.logger] - optional logger, see _flow/logger.js | ||
| * @param {object} [options.failClosed] - this determines the startup failure mode of RemoteBaggageRestrictionManager. |
There was a problem hiding this comment.
It isn't easy to grasp whatfailClosed does simply by looking at the name.
I suggest renaming it to something like the following:
startupFailurePolicy: allow | deny
There was a problem hiding this comment.
I thought so too, but according to Yuri: "it's a very common engineering term and you should be ashamed for not knowing it"
There was a problem hiding this comment.
https://www.google.com/search?q=fail+open+fail+closed. Widely used in security.
But I agree that it's not the best name to use for the options, I was simply referring to the principle. A property with allow | deny values sgtm. Is it only on startup? I'd be more specific with the name.
| _logger: Logger; | ||
| _metrics: Metrics; | ||
|
|
||
| _refreshInterval: number; |
There was a problem hiding this comment.
Could you append the unit to the key, something like refreshIntervalMs or so
| * @param {number} [options.port] - port for jaeger-agent for BaggageRestrictionManager endpoint | ||
| * @param {function} [options.onRestrictionsUpdate] | ||
| */ | ||
| constructor(serviceName: string, options: any = {}) { |
There was a problem hiding this comment.
How does the caller know that startup of the RemoteBaggageRestrictionManager is successful?
I see that it maintains an initialized bool, but I feel a callback might be better.
There was a problem hiding this comment.
The user can decide up front what to do incase of initialization failure via the failclosed variable. I dont think we need a callback.
There was a problem hiding this comment.
Yes, but what if I want custom behavior. For e.g., I don't want the service to startup if this condition is not met.
| @@ -106,6 +122,9 @@ export default class Span { | |||
| if (this.getBaggageItem(normalizedKey)) { | |||
| logs['override'] = 'true'; | |||
| } | |||
| if (truncated) { | |||
| logs['truncated'] = 'true'; | |||
There was a problem hiding this comment.
Is there a better way of capturing this data?
With multiple baggage items, it might be difficult to figure out which one was truncated
There was a problem hiding this comment.
Each baggage item gets its own set of logs. If we were to dump everything into the same logs, then there would be trouble. I think this current approach is fine.
| this._initialized = false; | ||
| this._onRestrictionsUpdate = options.onRestrictionsUpdate; | ||
|
|
||
| this._refreshBaggageRestrictions(); |
There was a problem hiding this comment.
Why does this not respect the initial delay?
There was a problem hiding this comment.
the baggage restrictions cache should be initialized (or at least attempted) before this constructor completes. This way when the tracer is initialized, users can start adding baggage right away rather than having to wait for at most 60 seconds for the initial delay.
There was a problem hiding this comment.
Why though?
Without making this a blocking call, there is no guarantee that the request was completed.
| } | ||
|
|
||
| _updateRestrictions(restrictions: Array<BaggageRestriction>) { | ||
| let restrictionsMap: { [key: string]: number } = {}; |
There was a problem hiding this comment.
Does it make more sense to have the agent return a map?
There was a problem hiding this comment.
sheeeeet, yeah this is a good idea... My only gripe with this approach is that in the idl, I defined it as: https://github.com/uber/jaeger-idl/blob/master/thrift/baggage.thrift which I prefer over map<string, i32> because it's harder to tell what the variables mean without having a comment describing it.
Now the debate becomes lets leave the thrift idl as is but have the agent convert the array into a map before returning it. This does make sense but again you have that issue where as the consumer of the json, you dont know what the variables in the map symbolizes. By returning it as json array instead of map, each variable is keyed correctly so you know what they symbolize. wdyt?
There was a problem hiding this comment.
re: thrift idl:
Thrift supports typedefs, so you can have a map<BaggageKey, BaggageValue>
There was a problem hiding this comment.
I prefer wire format to be lists, not maps. But thrift format should be hidden behind an interface, it's fine to return a map from the interface.
There was a problem hiding this comment.
@yurishkuro Why do you prefer the wire format to be a list even when holding map data? What advantages does it have to simply having it as a map?
There was a problem hiding this comment.
extensibility - jaegertracing/jaeger-idl#30
* Skeleton: Add basic-tracer package * remove codecov for now
No description provided.