Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

What do we want to do with Money? #138

Open
ptravers opened this issue Oct 4, 2020 · 12 comments
Open

What do we want to do with Money? #138

ptravers opened this issue Oct 4, 2020 · 12 comments
Assignees

Comments

@ptravers
Copy link
Collaborator

ptravers commented Oct 4, 2020

I created this issue to have conversation over what we want to do with Money. To be more specific, the expected output of this issue is hopefully a clear direction as to what the library should become.

My personal view is that we should help our users migrate to Open Telemetry

@HaloFour
Copy link
Collaborator

HaloFour commented Oct 4, 2020

Thanks for opening this issue! I was already exploring what would be involved in making Money an implementation of OpenTelemetry by either implementing the OpenTelemetry API interfaces directly or by providing a wrapper type (probably the latter, less disruptive). I've got a small list of changes to Money that I think would be necessary to support the additional functionality of OpenTelemetry. I can post them later today or tomorrow.

@HaloFour
Copy link
Collaborator

HaloFour commented Oct 5, 2020

Changes to spans:

  1. Allow updating of span name
  2. Allow stopping a span with an end timestamp
  3. Expand supported Note[T] types to include String, Long, Double and Boolean arrays.
  4. Add concept of Events that can have a name, timestamp and map of Note[T]

There is also a bit more state in an OTel Span that we might need to add:

  1. Span kind (internal, server, client, etc.)
  2. Span status (canonical code + description)
  3. Trace flags (whether or not the span is sampled)
  4. Trace state (used to propagate custom state in the tracestate header)

Some of this could probably be stored as notes but it might get a bit noisy.

I am still digging around in the OTel APIs and may suss out a few more things. @ptravers If you know of anything I'm missing please let me know.

@pauljamescleary
Copy link
Member

I think this makes sense. Adding methods to the API are good.

I wonder if money can directly implement any open-telemetry API / interface(s) as well? In other words, abandon what money has in it's own java interfaces, and substitute with what is in open-telemetry, updating the money-core as needed. Granite I have not looked at all at any of this stuff so this is spitballing a bit.

@HaloFour
Copy link
Collaborator

HaloFour commented Oct 6, 2020

I wonder if money can directly implement any open-telemetry API / interface(s) as well? In other words, abandon what money has in it's own java interfaces, and substitute with what is in open-telemetry, updating the money-core as needed. Granite I have not looked at all at any of this stuff so this is spitballing a bit.

Certainly possible. There's a lot of overlap between the interfaces and with the exception of the handful of concepts that Money doesn't (yet) support that I mentioned above the members do more or less map one-to-one with Money APIs. That may be a bit confusing to users, though, if they see both end and stop methods for stopping spans, and record and addAttribute methods for adding notes to spans. Not to mention the name collisions between Tracer, Span and maybe a few others. But perhaps these are minor issues, or perhaps we move away from the Money APIs and towards the OTel APIs instead?

@ptravers
Copy link
Collaborator Author

ptravers commented Oct 6, 2020

I would be more in favour of having a breaking release and updating the money interface to match OTel. I think the main issue we will have is making sure people can continue to get the data as structured logs and that the log structure doesn't change. We don't want to break everyone's dashboards.

@ptravers
Copy link
Collaborator Author

ptravers commented Oct 6, 2020

We should though have a release where we deprecate stop and add end

@HaloFour
Copy link
Collaborator

HaloFour commented Oct 6, 2020

I can start on a WIP PR that has the Money interfaces extend from the OTel interfaces and see where that lands.

More spaghetti at the wall, maybe we can keep the Money members intact but deprecated as a first cut release and then remove those members entirely in a second release?

@ptravers
Copy link
Collaborator Author

ptravers commented Oct 6, 2020

Yeah definitely! Sorry that's what I was trying to say poorly in my comments

@ptravers
Copy link
Collaborator Author

ptravers commented Oct 6, 2020

Thank you @HaloFour for stepping up and picking up this work

@HaloFour
Copy link
Collaborator

The major work to wrap the Money API in an OpenTelemetry API is complete. I have a few additional PRs up to improve the integration:

  1. Format span IDs as hex in the logging span handlers, to match the OTel ID formats: Add config to format span IDs as hex in logging span handlers #146
  2. Wrap OTel SpanProcessors as Money SpanHandlers so that Money can use OTel exporters like ZipKin or Jaeger: Adds project to wrap OTel SpanProcessor/SpanExporter as Money SpanHandler #147

I'm also looking at changes to propagate trace flags (sampled) and trace state through the Money SpanId/SpanContext so that Money can propagate them between upstream and downstream systems. This will also impact Money Formatters which will have to convey that state. I was considering an adapter between Money Formatters and OTel TextMapPropagators so that we can use off-the-shelf propagators rather than maintaining our own. OTel already has implementations for ZipKin and TraceContext, as well as a few others. We could even make formatters configurable like handers are.

Once SpanId can track sampling flags adding a configurable sampler to Money for new traces should be trivial and SpanHandlers can be written to respect those flags.

@HaloFour
Copy link
Collaborator

HaloFour commented Oct 21, 2020

I believe I've completed everything I mentioned above:

  1. Money now supports configurable Formatters for propagating trace/span state between services. The order can be specified in the configuration which prioritizes which headers are interpreted.
  2. Money offers a shim Formatter that wraps the OTel TextMapPropagator interface so that Money can interpret trace/span between services using a variety of other tracing technologies (via money-otel-formatters):
    • ZipKin (both multiple and consolidated header formats)
    • Jaeger
    • Lightstep
    • AWS X-Ray
    • W3C Trace Context
  3. Money will interpret the sampling and trace state and capture it in the SpanId of the remote span and all child spans so that it can be interpreted in any SpanHandler as well as propagated upstream
  4. Money offers a shim SpanHandler that wraps the OTel SpanProcessor/SpanExporter interface so that Money can export traces to any OpenTelemetry-compatible exporter, with the two currently supported:
    • ZipKin (via money-otel-zipkin-exporter)
    • Jaeger (via money-otel-jaeger-exporer)

Possible additional projects:

  1. Add sampling configuration so that Money will set the sampling state for any root span it creates
  2. Expand the MoneyTrace headers to propagate the sampling state (and additional state)
    • The Scala implementation of Money is flexible here, existing implementations will only parse the first three name-value pairs of a semicolon-delimited value. However, the Go implementation is inflexible and doesn't understand a header with more than 3 pairs.
  3. Bring in additional OTel exporters:
    • Logging
    • OTLP (OpenTelemetry collector)
  4. Test Money with OpenTelemetry Instrumentation and make any necessary changes needed to work with it
  5. ... ?
  6. Profit!

@HaloFour
Copy link
Collaborator

I'm creating another branch in which I plan on following the snapshot releases of OpenTelemetry API 0.10.0 and working on any of the breaking changes. So far it doesn't seem too bad. A few things renamed, a few things moved around. The biggest changes are around span context as the GRPC Context has been replaced with their own implementation. A lot of the Span methods also now return the Span so I need to wrap them in the covariant interface and return the instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants