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: histogram aggregator lastUpdateTime #1567

Merged

Conversation

AndrewGrachov
Copy link
Contributor

Which problem is this PR solving?

Timestamp in prometheus export is not updated over time

Short description of the changes

  • Did as every other aggregator toPoint methods do - update _lastUpdateTime on update call

@AndrewGrachov
Copy link
Contributor Author

tests are on the way

@codecov
Copy link

codecov bot commented Oct 2, 2020

Codecov Report

Merging #1567 into master will decrease coverage by 0.39%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1567      +/-   ##
==========================================
- Coverage   92.93%   92.54%   -0.40%     
==========================================
  Files         156      129      -27     
  Lines        4870     3406    -1464     
  Branches      988      625     -363     
==========================================
- Hits         4526     3152    -1374     
+ Misses        344      254      -90     
Impacted Files Coverage Δ
...emetry-metrics/src/export/aggregators/Histogram.ts 100.00% <100.00%> (ø)
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 80.00% <0.00%> (-20.00%) ⬇️
...es/opentelemetry-tracing/src/MultiSpanProcessor.ts
packages/opentelemetry-plugin-http/src/utils.ts
packages/opentelemetry-tracing/src/Tracer.ts
packages/opentelemetry-tracing/src/Span.ts
packages/opentelemetry-node/src/config.ts
packages/opentelemetry-plugin-https/src/https.ts
packages/opentelemetry-plugin-http/src/http.ts
...elemetry-tracing/src/export/SimpleSpanProcessor.ts
... and 19 more

@AndrewGrachov
Copy link
Contributor Author

test is ready

@AndrewGrachov
Copy link
Contributor Author

fixed. probably worth to squash on merge

@AndrewGrachov AndrewGrachov changed the title chore: fix histogram aggregator update time fix: fix histogram aggregator update time Oct 3, 2020
@AndrewGrachov AndrewGrachov changed the title fix: fix histogram aggregator update time fix: histogram aggregator lastUpdateTime Oct 3, 2020
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

@obecny obecny merged commit 60d4dab into open-telemetry:master Oct 5, 2020
@obecny obecny added the bug Something isn't working label Oct 5, 2020
@AndrewGrachov AndrewGrachov deleted the fix-histogram-update-time branch October 6, 2020 04:17
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
…e instead (open-telemetry#1567)

* feat(koa): Skip update HTTP's span name and update RpcMetadata's route instead

* make the logic of rpcMetadata.route the same as previously

* Remove unused variable

---------

Co-authored-by: Haddas Bronfman <85441461+haddasbronfman@users.noreply.github.com>
Co-authored-by: Amir Blum <amirgiraffe@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants