-
Notifications
You must be signed in to change notification settings - Fork 104
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
1.0.0-rc.1 - split metrics out to experimental applications #215
1.0.0-rc.1 - split metrics out to experimental applications #215
Conversation
Codecov Report
@@ Coverage Diff @@
## main #215 +/- ##
==========================================
- Coverage 35.78% 34.96% -0.82%
==========================================
Files 64 37 -27
Lines 3365 3037 -328
==========================================
- Hits 1204 1062 -142
+ Misses 2161 1975 -186
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
8596965
to
7b6a640
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figure the risk of keeping the same module names but a different app is that during a transition period, the experimental stuff may be brought in through transitive dependencies of some kind and then release-building will complain of module names clashing.
This however still seems nicer to fix (by excluding apps in releases) than having to rewrite all modules for using a different name (i.e. otelx_meter
) since it would allow drop-in replacements where possible while only changing the app name (the effect might be unpredictable with wild API changes but not much that could be done with that either way). There's no super clear way to handle this in tests outside of overriding the deps out of scope.
That being said, while that works reasonably well with Erlang, we should just go with what is the least surprise for Elixir folks, but I'm not sure I'm good to give advice there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of prepping the main packages for a 1.0.0 RC, this looks fine to me.
As for the experimental parts, my gut feeling is that any use of experimental stuff would be an "at your own risk" situation where the user should expect to have to make changes to that in the future. That said, I think it would be nice to keep all the names looking like the normal OpenTelemetry
modules instead of OpenTelemetryExperimental
, so that the transition to a real release would potentially be easier on both us as maintainers and on users/integration authors who had been using the experimental versions. One concern I can think of are that if we have Application
config settings, we may want to have them under the _experimental
namespace so that Elixir appropriately shows warnings when you have configurations for an app that you aren't including (better DevEx when they miss adding the dependency and don't understand why it's not working).
@@ -23,17 +23,18 @@ jobs: | |||
- uses: actions/checkout@v2 | |||
- name: Run Collector | |||
run: docker-compose up -d | |||
- uses: gleam-lang/setup-erlang@v1.0.0 | |||
- uses: erlef/setup-elixir@v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended to also use the Elixir image? Obviously it will also have Erlang available, but just wanted to make sure this was intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, there isn't a erlef/setup-erlang
yet I don't think.
I know we discussed shipping metrics and logging as separate packages, so I wonder if we need the experimental at all or just have a metrics package which falls under the OpenTelemetry namespace. Then as we get the metrics to 1.0, that would just roll into the main package. |
7b6a640
to
a9493f2
Compare
@bryannaegele I think having a single experimental application/package is simpler, instead of having one for metrics and logs and then having to deprecate them in hex. It doesn't make moving them to the main applications any easier as far as I can tell. |
@GregMefford definitely agree that configuration for metrics and logs (signals that are still experimental) should be under the experimental application. |
Yeah, we talked ourselves to the same place :) I did take a look a the namespacing for Elixir though and having it under |
This is more work then I realized.. The elixir api stuff I'm unsure about module names for. Like should everything be
OpenTelemetryExperimental.*
or justOpenTelemetry
since that is technically legal...This will bring us to an actual 1.0.0 RC release (0.6.x is the same api but still has metrics still in it).
Opening as a draft because I'm sure there are changes still needed in some of the Elixir modules and I just don't have the time/patience to deal with it right now, but want to get reviews anyway.