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

Telemetry Metrics #214

Merged
merged 21 commits into from
May 3, 2024
Merged

Telemetry Metrics #214

merged 21 commits into from
May 3, 2024

Conversation

MichaelGHSeg
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.10%. Comparing base (79cfb46) to head (21398d6).
Report is 1 commits behind head on main.

❗ Current head 21398d6 differs from pull request most recent head edeb8c8. Consider uploading reports for the commit edeb8c8 to get more accurate results

Additional details and impacted files
@@              Coverage Diff              @@
##               main     #214       +/-   ##
=============================================
- Coverage     78.65%   46.10%   -32.55%     
+ Complexity      502       14      -488     
=============================================
  Files            78        8       -70     
  Lines          6588      874     -5714     
  Branches        833       85      -748     
=============================================
- Hits           5182      403     -4779     
+ Misses          735      437      -298     
+ Partials        671       34      -637     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@wenxi-zeng wenxi-zeng left a comment

Choose a reason for hiding this comment

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

overall in good shape. just 4 things needs to be addressed before merged:

  1. possible concurrent issue on the telemetry queue
  2. consolidate 3 error reporting methods to 1 for simplicity
  3. better to have a lookup table for telemetry queue
  4. make Telemetry a Subscriber of Sovran

assertEquals(0, TelemetryQueueSize())
assertEquals(0,errors.size)
}

@Test
fun `Test increment with no tags`() {
Telemetry.start()
Telemetry.increment(Telemetry.INVOKE_METRIC, emptyMap())
Telemetry.increment(Telemetry.INVOKE_METRIC) { it.clear() }
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to clear. can just leave it with empty brackets. or make the tagBuilder nullable, so you don't have to pass anything

Copy link
Contributor Author

@MichaelGHSeg MichaelGHSeg May 2, 2024

Choose a reason for hiding this comment

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

It was in the test so I just figured I'd make it explicit about what it was expected to do, rather than minimize code. Just say the word if you'd rather it go away.

@MichaelGHSeg MichaelGHSeg merged commit 9c05e65 into main May 3, 2024
8 checks passed
@MichaelGHSeg MichaelGHSeg deleted the MichaelGHSeg/Telemetry branch May 3, 2024 17:47
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 this pull request may close these issues.

3 participants