[Feature Request] Store misc information within TraceContext which are not propagated

[Alon Bar-Lev]

Hi Adrian,

Continuing discussion from [1].

Thank you so much of making [2] change, it is good enough to get me going.

The minor gaps I still see are:

Ability to attach "object" and not only plain strings.
Ability to attach arbitrary keys, can be solved by having the opposite
BitSet... mark these that are regular so that everything else is
redacted.

Thank you again.,
Alon

[1] https://github.com/openzipkin/brave/issues/929
[2] https://github.com/openzipkin/brave/pull/931/files

[Adrian Cole]

Gotcha and thanks for all the feedback.

Bear in mind that ExtraFieldsPropagation is not the same as a more
fully featured local propagation tool which has been attempted in the
past but not complete [1]. Also, we normally use the "rule of three"
[2] on things, which optimizes maintenance towards a balance of mutual
desire (feature is wanted and code is maintained). Features add tax to
the project so we need to make sure code counts and is relevant.

> Ability to attach "object" and not only plain strings.

Examples? What would you want to put in as an object and what
integration wants an object as opposed to a string?

> Ability to attach arbitrary keys, can be solved by having the opposite
> BitSet... mark these that are regular so that everything else is
> redacted.

It isn't obvious why it would make sense to optimize a feature for
header propagation to not propagate :P Seems a slippery slope towards
the other feature [1], but we do want to understand in case it isn't

[1] https://github.com/openzipkin/brave/pull/577
[2] https://github.com/openzipkin/brave/blob/master/HACKING.md

[Alon Bar-Lev]

On Wed, Jun 26, 2019 at 5:28 PM Adrian Cole <adrian...@gmail.com> wrote:
>
> Gotcha and thanks for all the feedback.
>
> Bear in mind that ExtraFieldsPropagation is not the same as a more
> fully featured local propagation tool which has been attempted in the
> past but not complete [1]. Also, we normally use the "rule of three"
> [2] on things, which optimizes maintenance towards a balance of mutual
> desire (feature is wanted and code is maintained). Features add tax to
> the project so we need to make sure code counts and is relevant.
>
> > Ability to attach "object" and not only plain strings.
> Examples? What would you want to put in as an object and what
> integration wants an object as opposed to a string?

Well, I thought of holding a reference to an object that may be
complex for the scope decorator to consume... or an generic object in
which the toString() will be used to produce the actual content of the
field when propagated... It is only a matter of holding object
reference instead of string to make it happen... but nevertheless it
is not that important.

> > Ability to attach arbitrary keys, can be solved by having the opposite
> > BitSet... mark these that are regular so that everything else is
> > redacted.
> It isn't obvious why it would make sense to optimize a feature for
> header propagation to not propagate :P Seems a slippery slope towards
> the other feature [1], but we do want to understand in case it isn't

I believe [1] is important as well, but not entirely related to our
discussion... all I am saying is that everything that is added as
addField is for sure propagated... if we have bitmask of these we can
avoid declaring the redacted explicitly and just set them, so that
factoryBuilder.addField(key) will mark key in bitmask, but will allow
to add additional fields into the extra() which will not be
propagated. I try to understand the optimization of using indexes
instead of plain maps... you must know something I don't if you
implemented this way...

In other words, I would have expected to declare explicitly only the
propagated fields, while allowing to add additional fields without
prior deceleration, if this is against an optimization technique you
may have than I can understand why it cannot be implemented.

[Adrian Cole]

> In other words, I would have expected to declare explicitly only the
> propagated fields, while allowing to add additional fields without
> prior deceleration, if this is against an optimization technique you
> may have than I can understand why it cannot be implemented.

The feature was designed for remote propagation. It would be strange
to have so many fields that whitelisting became a problem or something
to optimize internal over (that's why I asked for examples).
Nevertheless, such a whitelist builder would be possible externally.

The opt-out of remote propagation is an edge case, so it wouldn't be
something we would change the design over. OTOH, you did ask a good
question, which is what is the rationale for maps vs the index-based
approach. It is about memory overhead and performance. Everything
added t the context adds weight and I would argue that context
tracking and propagation is the largest source of overhead in tracing.

You can look at the difference between using even simple propagation
fields vs a few in ExtraFieldPropagationBenchmarks and if you like you
can change to a map based approach to see which fares better.
Index-based has a lot of advantages.

Performance centrism is also why we haven't chosen to become a user
api, rather an SPI to implement tracing instrumentation. For example,
the "extra" part was designed to allow folks to create their own
mechanisms should they want to warranty their own work. The entire
propagation system is wrappable or swappable, which allows folks to
monkey with internals until features are popular and relevant enough
to warrant taking efforts from elsewhere..

[Alon Bar-Lev]

Thank you for your detailed response!
The patch you prepared is good enough for our use case, once you will
release a new version I will submit a patch to sleuth.