search

beam-telemetry / telemetry_metrics_prometheus_core

Core Prometheus Telemetry.Metrics Reporter package for telemetry_metrics_prometheus
Apache License 2.0
35 stars 30 forks source link

Switched to handle_continue and added flag to start synchronously #33

Closed akoutmos closed 3 years ago

akoutmos commented 4 years ago

The purpose of this PR is two fold.

One is to leverage the GenServer handle_continue callback as to guarantee that the initialization steps for the GenServer are the first things that take place upon spawning the process. I ran into some issues where telemetry_metrics_prometheus_core was being started as part of a supervision tree and I was getting inconsistent results on app start.

The second change is to add a flag to either init the process in the init function (in turn blocking the rest of the supervision tree from starting) or leveraging the handle_continue callback. The reason for this change is that if the supervision tree is allowed to continue without metrics being in place, you have a short window of time where you can miss telemetry events being emitted. The change currently has the default of using the handle_continue callback so the process behaviour is in line with what is in master today.

Thanks in advance for the review :)

bryannaegele commented 4 years ago

Hey @akoutmos. The only issue I see here is that this would necessitate dropping support for OTP 20. Let's leave this open for now.

I'm not strongly opposed to bumping the minimum version but OTP 20 is supported through Elixir 1.9 and I'm hesitant to raise it just to add this option, but this would be a good addition as part of a larger update taking advantage of more OTP 21 features.

akoutmos commented 4 years ago

Hey @bryannaegele! Totally understandable on the OTP 20 front. I can rewrite the async/non_async flag stuff to be done the way it was before versus using a handle_continue if that's ok with you?

bryannaegele commented 3 years ago

Hey @bryannaegele! Totally understandable on the OTP 20 front. I can rewrite the async/non_async flag stuff to be done the way it was before versus using a handle_continue if that's ok with you?

Yeah, that would be totally fine. Just be sure to add documentation for it and test coverage.

akoutmos commented 3 years ago

@bryannaegele This one should be good to go now! Thanks in advance for the review :).

akoutmos commented 3 years ago

Added some docs in my latest commit. Hopefully the additional context is useful for future readers :)

akoutmos commented 3 years ago

Friendly bump :). Was there anything else that needed cleaning up for this PR?

bryannaegele commented 3 years ago

Friendly bump :). Was there anything else that needed cleaning up for this PR?

Sorry. Just a lot going on. :)

bryannaegele commented 3 years ago

Oh! Actually, @akoutmos can you re-open a PR with this pointed at main? We aren't using master any longer.

akoutmos commented 3 years ago

It has been done! https://github.com/beam-telemetry/telemetry_metrics_prometheus_core/pull/39 Thanks again :)