-
Notifications
You must be signed in to change notification settings - Fork 33
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
Comments
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. |
Changes to spans:
There is also a bit more state in an OTel Span that we might need to add:
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. |
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. |
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 |
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. |
We should though have a release where we deprecate |
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? |
Yeah definitely! Sorry that's what I was trying to say poorly in my comments |
Thank you @HaloFour for stepping up and picking up this work |
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:
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. |
I believe I've completed everything I mentioned above:
Possible additional projects:
|
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. |
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
The text was updated successfully, but these errors were encountered: