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

Added BatchSpanProcessor metrics on drop and export span #635

Merged

Conversation

mamunto
Copy link
Contributor

@mamunto mamunto commented Nov 11, 2024

Added BatchSpanProcessor metrics on drop and export span

Copy link
Contributor Author

@mamunto mamunto left a comment

Choose a reason for hiding this comment

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

I'm following Java SDK implementation from here

@mamunto mamunto force-pushed the mdalmamun/add-batch-span-processor-metrics branch from d910005 to fd4b7e7 Compare November 18, 2024 15:13
@bryce-b bryce-b self-requested a review November 19, 2024 22:47
@bryce-b
Copy link
Member

bryce-b commented Nov 19, 2024

Looks good, I'll approve it after you add the // TODO: Record a gauge for referenced spans. if you want to do that.

@mamunto mamunto force-pushed the mdalmamun/add-batch-span-processor-metrics branch from 4522a0b to b41c887 Compare November 20, 2024 21:42
Comment on lines +146 to +157
deinit {
// Cleanup all gauge observer
self.queueSizeGauge?.close()
self.spanGaugeObserver?.close()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clean up here cc: @bryce-b

Copy link
Member

@nachoBonafonte nachoBonafonte left a comment

Choose a reason for hiding this comment

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

Sorry for being late, but I have some questions about the new protocol functions and how they match in the API/SDK

@mamunto
Copy link
Contributor Author

mamunto commented Nov 21, 2024

Sorry for being late, but I have some questions about the new protocol functions and how they match in the API/SDK

I followed exactly how it defined in the Java SDK. We can discuss more today's session

@mamunto mamunto force-pushed the mdalmamun/add-batch-span-processor-metrics branch from b41c887 to 6e92546 Compare November 21, 2024 22:03
@nachoBonafonte
Copy link
Member

nachoBonafonte commented Nov 22, 2024

LGTM, thanks for addressing all the feedback. I will let @bryce-b merge if he considers it ready also.

@bryce-b bryce-b merged commit 32b7b1e into open-telemetry:main Nov 22, 2024
6 checks passed
Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 78.88889% with 19 lines in your changes missing coverage. Please review.

Project coverage is 67.54%. Comparing base (7bad8ae) to head (675694f).
Report is 42 commits behind head on main.

Files with missing lines Patch % Lines
...ySdk/Trace/SpanProcessors/BatchSpanProcessor.swift 78.88% 19 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #635      +/-   ##
==========================================
- Coverage   67.89%   67.54%   -0.35%     
==========================================
  Files         344      352       +8     
  Lines       15169    15873     +704     
==========================================
+ Hits        10299    10722     +423     
- Misses       4870     5151     +281     

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


🚨 Try these New Features:

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