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

add metrics collector for BEAM VM #299

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RoadRunnr
Copy link

No description provided.

@bryannaegele
Copy link
Collaborator

I'm not sure that this doesn't belong in the sdk since it's vm runtime. @tsloughter thoughts? I'm not sure how this is being handled in the other languages. I do know the otel collector receivers just hook into existing APIs of the thing they're integrated with and are configurable.

Some general notes on first glance, forgive me if I misinterpreted something. I see that you relied heavily on prior art but I believe a fair amount of it doesn't comply with otel requirements.

  1. The naming here does not comply with semantic conventions for runtime names. Please review and adjust according to https://opentelemetry.io/docs/specs/semconv/general/metrics/
  2. I think some units will need to be adjusted. Review the conventions and spec carefully.
  3. How is filtering handled? Is that handled someplace else? In general, defaults are set for each available metric in a receiver, for instance, with the user having the ability to opt in and out of the defaults.

@RoadRunnr
Copy link
Author

@bryannaegele

The naming here does not comply with semantic conventions for runtime names. Please review and adjust according to https://opentelemetry.io/docs/specs/semconv/general/metrics/

The intention was to keep the metric names compatible with VM collectors from prometheus.erl. That way the OTel instrumentation would be a simple drop-in replacement for projects that currently use prometheus.erl and Grafana dashboards from there could be reused with minimum changes.

However, I do like the semantic conventions from OTel better than the old names. So, I'll change to them.

How is filtering handled? Is that handled someplace else?

I was under the impression that filtering would always be handled in a collector and not in the metric. Can you point me to example (even in anther language) where filtering is done in the meter?

@bryannaegele
Copy link
Collaborator

I don't have the time or context for that, so relying on you and Tristan to do that legwork ;)

Meter, 'erlang.vm.statistics.garbage_collection_words_reclaimed',
#{description => <<"Garbage collection: words reclaimed.">>})],

otel_meter:register_callback(Meter, Instruments, fun vm_stats/1, []),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops, completely missed these were created without callbacks, haha.

So yea, this is good.

But still I'd suggest grouping them so they aren't all read even if you only want certain of them? What do you think?

@tsloughter
Copy link
Member

@bryannaegele it looks like Java has their's in contrib, so probably belongs here. I'd have ben fine either way

@bryannaegele
Copy link
Collaborator

Cool.

What do y'all think of making the prefix be beam rather than erlang.vm? I don't think the double-namespace provides anything and it provides better specificity imo.

@RoadRunnr
Copy link
Author

RoadRunnr commented Apr 3, 2024

@bryannaegele @tsloughter updated:

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

Successfully merging this pull request may close these issues.

None yet

3 participants