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

fix(exporter-prometheus): add appendTimestamp option to ExporterConfig #1697

Merged
merged 12 commits into from
Dec 14, 2020
Merged

fix(exporter-prometheus): add appendTimestamp option to ExporterConfig #1697

merged 12 commits into from
Dec 14, 2020

Conversation

antoniomrfranco
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • This PR adds the appendTimestamp option to ExporterConfig. This option is used to control whether the timestamp will be included when exporting the metric or not. The default behavior was not changed, i.e., appendTimestamp defaults to true.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 24, 2020

CLA Signed

The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Nov 24, 2020

Codecov Report

Merging #1697 (0749345) into master (e70a7c8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1697   +/-   ##
=======================================
  Coverage   92.17%   92.17%           
=======================================
  Files         167      167           
  Lines        5594     5597    +3     
  Branches     1191     1193    +2     
=======================================
+ Hits         5156     5159    +3     
  Misses        438      438           
Impacted Files Coverage Δ
...etry-exporter-prometheus/src/PrometheusExporter.ts 92.18% <100.00%> (+0.38%) ⬆️

Signed-off-by: Antonio Franco <[email protected]>
Signed-off-by: Antonio Franco <[email protected]>
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Welcome to the project :). LGTM.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm, just last minor thing

@antoniomrfranco
Copy link
Contributor Author

It seems that the lint check broke after I updated my branch merging master.

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Looks fine but why is this configurable in the first place? Shouldn't we always append the timestamp?

@dyladan dyladan added the enhancement New feature or request label Dec 1, 2020
@antoniomrfranco
Copy link
Contributor Author

Looks fine but why is this configurable in the first place? Shouldn't we always append the timestamp?

Hi @dyladan. I don't think we should always append the timestamp. Actually, in Prometheus docs, they say that

you should not set timestamps on the metrics you expose, let Prometheus take care of that. If you think you need timestamps, then you probably need the Pushgateway instead.

So I think that timestamp is only required when you have a Pushgateway or a federated architecture (e.g., Prometheus pushing metrics to Cortex).

@dyladan
Copy link
Member

dyladan commented Dec 1, 2020

In that case, should we remove the timestamp option? Sounds like prometheus itself doesn't recommend its use.

@obecny obecny merged commit e032feb into open-telemetry:master Dec 14, 2020
@antoniomrfranco antoniomrfranco deleted the exporter-prometheus-fix-append-timestamp branch December 30, 2020 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants