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

Change processedSpans&processedLogs labels #5631

Closed
wants to merge 2 commits into from

Conversation

JoeCqupt
Copy link

when i use BatchLogRecordProcessor/BatchSpanProcessor.
i want to know dropped ratio and exported success ratio and exported fail ratio and etc. but exist metrics can not compute those.

i want achieve this goal by updating processedSpans&processedLogs labels .

dropped ratio: dropped/processed
exported success ratio: exported/(processed - dropped)
exported fail ratio: 1 - exported/(processed - dropped)

@JoeCqupt JoeCqupt requested a review from a team July 17, 2023 05:39
@JoeCqupt JoeCqupt changed the title update processedSpans&processedLogs labels Change processedSpans&processedLogs labels Jul 17, 2023
@JoeCqupt JoeCqupt force-pushed the main branch 2 times, most recently from b006926 to d9461d9 Compare July 17, 2023 06:53
@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (b0c5c04) 91.32% compared to head (2749de9) 91.31%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5631      +/-   ##
============================================
- Coverage     91.32%   91.31%   -0.01%     
+ Complexity     4979     4978       -1     
============================================
  Files           554      554              
  Lines         14731    14739       +8     
  Branches       1376     1376              
============================================
+ Hits          13453    13459       +6     
  Misses          884      884              
- Partials        394      396       +2     
Impacted Files Coverage Δ
...metry/sdk/logs/export/BatchLogRecordProcessor.java 91.03% <100.00%> (-0.46%) ⬇️
...telemetry/sdk/trace/export/BatchSpanProcessor.java 94.63% <100.00%> (+0.14%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JoeCqupt
Copy link
Author

@jkwatson @jack-berg PTAL

@jkwatson
Copy link
Contributor

This removes an existing attribute, correct? I'm worried it will break existing dashboards if we get rid of an attribute. @jack-berg is there a spec for these metrics yet?

@JoeCqupt
Copy link
Author

this modification is incompatible。 If you think compatibility is necessary, I can keep the original label and then add new labels

@@ -45,8 +49,8 @@ public final class BatchLogRecordProcessor implements LogRecordProcessor {
BatchLogRecordProcessor.class.getSimpleName() + "_WorkerThread";
private static final AttributeKey<String> LOG_RECORD_PROCESSOR_TYPE_LABEL =
AttributeKey.stringKey("logRecordProcessorType");
private static final AttributeKey<Boolean> LOG_RECORD_PROCESSOR_DROPPED_LABEL =
AttributeKey.booleanKey("dropped");
private static final AttributeKey<String> LOG_RECORD_PROCESS_STATUS_LABEL =
Copy link
Contributor

Choose a reason for hiding this comment

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

Strongly recommend avoiding using the term "label" to refer to OpenTelemetry attributes. Let's try and be consistent in our naming. 🙏🏻

@jack-berg
Copy link
Member

This is being actively discussed in open-telemetry/semantic-conventions#184. In order to reduce churn, I'm inclined to hold any changes to these metrics until a new set of experimental semantic conventions lands.

@JoeCqupt would you be able to comment over on that PR with your use case / requirements?

@JoeCqupt JoeCqupt closed this Nov 20, 2023
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.

4 participants