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(sdk-metrics): ignore invalid metric values #3988

Merged
merged 5 commits into from
Aug 1, 2023

Conversation

legendecas
Copy link
Member

Which problem is this PR solving?

Ignore invalid metric values.

Fixes #3985

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • SyncInstruments
  • ObservableResult.

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added

@legendecas legendecas force-pushed the metrics/record-type branch from b18e4ca to fc10848 Compare July 13, 2023 07:19
@legendecas legendecas marked this pull request as ready for review July 13, 2023 07:19
@legendecas legendecas requested a review from a team July 13, 2023 07:19
@legendecas legendecas added the sdk:metrics Issues and PRs related to the Metrics SDK label Jul 13, 2023
@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #3988 (af43971) into main (87fff2e) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head af43971 differs from pull request most recent head 7065a5c. Consider uploading reports for the commit 7065a5c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3988      +/-   ##
==========================================
+ Coverage   92.34%   92.36%   +0.02%     
==========================================
  Files         321      321              
  Lines        9249     9264      +15     
  Branches     1962     1968       +6     
==========================================
+ Hits         8541     8557      +16     
+ Misses        708      707       -1     
Files Changed Coverage Δ
packages/sdk-metrics/src/Instruments.ts 95.74% <100.00%> (+0.50%) ⬆️
packages/sdk-metrics/src/ObservableResult.ts 100.00% <100.00%> (+3.57%) ⬆️

@legendecas legendecas force-pushed the metrics/record-type branch from fc10848 to 85d6a01 Compare July 13, 2023 07:24
@pichlermarc pichlermarc added the bug Something isn't working label Jul 13, 2023
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Glad to have this fixed. Thank you for taking care of this. 🙂

Copy link
Member

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

Sorry on the delay, just needs a changelog conflict resolved 👍

@dyladan
Copy link
Member

dyladan commented Jul 26, 2023

looks like the node 8 backtests are failing for some reason but I can't see what that reason is. The tests have unexpected token { but the sources look ok to me. Anyone got any ideas?

@martinkuba
Copy link
Contributor

Looks like it's now failing on other PRs (and main as well). Some dependency must have changed.

@pichlermarc
Copy link
Member

looks like the node 8 backtests are failing for some reason but I can't see what that reason is. The tests have unexpected token { but the sources look ok to me. Anyone got any ideas?

@dyladan @martinkuba looks a dependency in istanbul was updated that causes it to not work with Node.js v8 anymore, see #4030

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working sdk:metrics Issues and PRs related to the Metrics SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Histogram.record() loses its mind if you pass a string in as the first argument.
6 participants