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

Are calls to add_distribution_value synchronous or asynchronous? Can they raise errors? #887

Closed
thbar opened this issue Sep 12, 2023 · 2 comments

Comments

@thbar
Copy link

thbar commented Sep 12, 2023

In:

An example of Ecto instrumentation is provided to show how to track pool statistics (idle_time etc) via calls to:

Appsignal.add_distribution_value("ecto.queue_time", System.convert_time_unit(queue, :native, :millisecond))

One must note, though, that telemetry handlers are run "inline", as explained here:

Handler code runs inline wherever events are emitted – in the same process emitting the events – which is the reason for those tight constraints around failure. The upside of that approach is that telemetry doesn’t involve message passing, which can be a problem with large enough data as context and/or volume of events. Reattaching failing event handlers automatically could be devastating to a systems stability, so this is not available.

I looked a bit at the code (which ends up in the NIF), and wondered:

  1. Are these add_distribution_value calls synchronous?
  2. Could they raise errors?

I know that an effort is planned to track those metrics as custom metrics here:

but in the mean time, since I will use the implementation provided in #318 (comment), I would like to understand the potential consequences.

Thanks!

@unflxw
Copy link
Contributor

unflxw commented Sep 12, 2023

Hi @thbar! TL;DR: It is safe to call add_distribution_value (or interface with AppSignal in other ways) from within a Telemetry handler. All operations involving the NIF are non-blocking.

  1. Are these add_distribution_value calls synchronous?

Yes, they're "synchronous", in that they synchronously communicate with the AppSignal extension via the NIF. But they're not "synchronous" in that they'd block until the value is sent to AppSignal, or anything like that. Communicating through the NIF takes a very short time. This is to say, this should not be a problem -- the value is merely queued in the extension, which will send it to AppSignal asynchronously.

(Our own instrumentations interface with the NIF from within Telemetry handlers -- see Ecto for example)

  1. Could they raise errors?

In practice, no. They could raise an error if they're not being invoked correctly -- say, if you were to pass a string as the distribution value instead of a double. As long as the right number of arguments are passed with the right types, no error will be raised.

While the integration's NIF interfaces with the AppSignal agent, which is not open source, the NIF bridge itself is publicly available, and you can take a look at the implementation for add_distribution_value.

That said, if an error was somehow raised, the consequence would be that Telemetry would detach the event handler that raised an error. This error would be logged, and your application would no longer emit this metric to AppSignal, but your application would not crash because of it.

@thbar
Copy link
Author

thbar commented Sep 12, 2023

Hi @unflxw! Thanks for the timely & thoughtful answer!

Yes, they're "synchronous", in that they synchronously communicate with the AppSignal extension via the NIF. But they're not "synchronous" in that they'd block until the value is sent to AppSignal, or anything like that.

This is what I hoped for, given the type of software 😄 thanks for the confirmation!

They could raise an error if they're not being invoked correctly -- say, if you were to pass a string as the distribution value instead of a double

Ok! I looked at the code before and indeed was wondering what would happen inside the NIF, would it block or not etc.

Many thanks, this is perfect and I can move forward with our implementation. Thank you!

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

2 participants