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

Remove prom-client from prometheus exporter #1039

Closed
dyladan opened this issue May 8, 2020 · 10 comments · Fixed by #1310
Closed

Remove prom-client from prometheus exporter #1039

dyladan opened this issue May 8, 2020 · 10 comments · Fixed by #1310
Assignees

Comments

@dyladan
Copy link
Member

dyladan commented May 8, 2020

On the long term i believe we should implement the serialization as text ourselves to avoid the hack i've made in 2).

There is a similar hack for counter. I think removing prom-client is probably a good idea in the end.

Originally posted by @dyladan in #1038 (comment)

The main advantage of the prom-client is that it does aggregations for you. Because we do aggregation in the SDK, we actually have to work around this "feature" to force the prom-client to use the actual values instead of trying to aggregate new exported values with old values.

In the current state of the code, the prom-client Counter is destroyed and re-created on each export, and there is a similar proposed "hack" for measure in #1038

@dyladan dyladan added the Discussion Issue or PR that needs/is extended discussion. label May 8, 2020
@dyladan
Copy link
Member Author

dyladan commented May 8, 2020

Also see #1032 (comment)

@dyladan
Copy link
Member Author

dyladan commented May 8, 2020

@astorm is this something you were thinking to handle or should I mark it as up for grabs?

@dyladan dyladan removed the Discussion Issue or PR that needs/is extended discussion. label May 8, 2020
@astorm
Copy link
Contributor

astorm commented May 8, 2020

@dyladan Let's mark it up for grabs for now -- I don't think I'd be able to come back to it promptly.

@dyladan dyladan added the up-for-grabs Good for taking. Extra help will be provided by maintainers label May 8, 2020
@mayurkale22
Copy link
Member

I think we should bring/inform this issue to a larger audience i.e. OT community or OT maintainers group.

@astorm
Copy link
Contributor

astorm commented May 8, 2020

@mayurkale22 Quick related question if you have the time -- Re: "this issue" -- do you mean the specific problem with the prom-client that @dyladan mentioned? Or the more general problem of "what should we do when a metric's incremented with an incomplete set of aggregated values"? Or some third thing that I don't quite see because I'm new to the project?

@vmarchaud
Copy link
Member

There are two issues right now:

  • Counter are exported with their value however prom-client expect us to increment/decrement it but we can't since we don't have the information at this level.
  • Some aggregator implemented in otel does the same thing as the one in prom-client so we receive aggregated-value in the exporter, which we cannot give directly to prom-client.

That's why it might be easier to just write someting (or use something that already exist) to just take the exported values and write them in the openmetrics spec than maintains "hacks" around prom-client

@dyladan
Copy link
Member Author

dyladan commented May 12, 2020

@vmarchaud I'm not sure if the openmetrics spec is useable in place of the prometheus exposition format. Even if prometheus does ingest it properly, there may be subtle differences. It may be worth creating an openmetrics exporter to be clear about the distinction.

edit: it looks like it is on their roadmap

@legendecas
Copy link
Member

I would like to work on this as part of #1278. I'm assigning myself.

@legendecas legendecas self-assigned this Jul 6, 2020
@vmarchaud
Copy link
Member

@legendecas I guess my PR (#1038) would be replaced by yours then ? The only thing that would be realy useful when rewriting the exporter is to use prometheus metric type (like histogram and summary)

@legendecas
Copy link
Member

@vmarchaud the prom-client part of the #1038 will be replaced by a new serializer, yet the choice on the aggregator mapping still has to be done.

@legendecas legendecas removed the up-for-grabs Good for taking. Extra help will be provided by maintainers label Jul 7, 2020
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue Dec 15, 2023
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

Successfully merging a pull request may close this issue.

5 participants