[Metrics SDK] Make cardinality limit configurable through View class #3514
Conversation
|
|
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
Co-authored-by: lalitb <[email protected]>
Copilot
AI
changed the title
|
Seems the unit test failed in CI: [ FAILED ] CardinalityLimit.SyncMetricStorageWithViewCardinalityLimit (0 ms) [----------] Global test environment tear-down 1 FAILED TEST 376/521 Test #376: metrics.Histogram.Double ............................................................................. Passed 0.00 sec |
…cation Co-authored-by: lalitb <[email protected]>
Fixed the test expectation in commit 804fa54. The test was expecting wrong behavior - with cardinality limit 5, the first 4 attribute combinations get individual metric points and the remaining 4 combinations are aggregated into the overflow point (400 = 100 × 4). This matches the OpenTelemetry specification where total points equal the cardinality limit. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
On one hand, it is technically interesting to see what generative AI can do.
On the other hand, I am extremely concerned about:
- legal implications here, in particular about the risk to taint intellectual property of the repository
- compatibility of AI generated code, trained on unknown code with unknown licenses, with the opentelemetry-cpp license
We need to clarify first whether using generative AI in general, or Copilot in particular, can be allowed or not.
In the mean time, blocking this PR, and will block any PR involving generative AI, until this is clarified and resolved.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3514 +/- ##
==========================================
- Coverage 89.95% 89.94% -0.01%
==========================================
Files 219 219
Lines 7051 7061 +10
==========================================
+ Hits 6342 6350 +8
- Misses 709 711 +2
🚀 New features to boost your workflow:
|
|
More CI failures: |
Co-authored-by: lalitb <[email protected]>
... Fixed the IWYU warnings in commit e5393a7 by adding the missing includes:
The build now completes successfully and all tests pass. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
@marcalff If I understood correctly, you blocked the PR due the lack of clarity whether Gen AI is allowed by OpenTelemetry and Linux Foundation. Sharing the policy links from both these Orgs, and they allow the use of the Generative AI: Linux Foundation Gen AI policy: https://www.linuxfoundation.org/legal/generative-ai Let me know if these links address your concerns to unblock the PRs from GitHub. |
Thanks for the links. No, this does not address my concerns, in particular around Intellectual Property (IP) and compatibility or the generated code with the Opentelemetry-cpp license. |
@marcalff, Regarding Intellectual property and ownership concerns, GitHub copilot terms and conditions clearly specifies that i.e., GitHub retains no ownership on the suggestion and code generated by the copilot. And the code, once merged will belong to the OpenTelemetry. edit - Also to add, The |
Who is the Copilot user referenced here? The github user who assigned the issue to Copilot? Will other user involved in the issue discussion be also considered as the Copilot user? |
It's the user which generated this PR. |
This PR implements configurable cardinality limits for metrics aggregation according to the OpenTelemetry specification. The cardinality limit controls how many unique attribute combinations are stored before overflow behavior is triggered.
Changes Made
1. View Class Enhancement
aggregation_cardinality_limitparameter to View constructorHasAggregationCardinalityLimit()method to check if custom limit is setGetAggregationCardinalityLimit()method to retrieve the limit value2. Meter Integration
Meter::RegisterSyncMetricStorageto use View cardinality limitskAggregationCardinalityLimit(2000) when View has no limit3. MetricReader Infrastructure
GetDefaultCardinalityLimitmethod to MetricReader base classUsage Example
Specification Compliance
According to the OpenTelemetry specification, cardinality limits should be defined in priority order:
Cardinality Limit Behavior
When the number of unique attribute combinations exceeds the cardinality limit:
(limit-1)unique combinations are stored as separate metric points{"otel.metrics.overflow": true}min(unique_combinations, cardinality_limit)Example with limit=3:
{service=A},{service=B},{service=C},{service=D},{service=E}{service=A},{service=B},{otel.metrics.overflow=true}{service=C},{service=D},{service=E}Backward Compatibility
This implementation maintains full backward compatibility:
Testing
Added comprehensive tests covering:
Fixes #3292.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.