[METRICS] Add tag to AggregationConfig for aggregation type validation by ThomsonTan · Pull Request #3732 · open-telemetry/opentelemetry-cpp

ThomsonTan · 2025-10-31T00:50:03Z

Fixes #3731

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2025-10-31T00:53:39Z

+      switch (aggregation_type_)
+      {
+        case AggregationType::kHistogram:
+          valid = (config_type == AggregationConfigType::kHistogram);
+          break;
+        case AggregationType::kBase2ExponentialHistogram:
+          valid = (config_type == AggregationConfigType::kBase2ExponentialHistogram);
+          break;
+        case AggregationType::kDefault:
+        case AggregationType::kDrop:
+        case AggregationType::kLastValue:
+        case AggregationType::kSum:
+          valid = (config_type == AggregationConfigType::kDefault);
+          break;


Allow histogram configs when aggregation type is default

The new validation in View now throws whenever AggregationType::kDefault is paired with a HistogramAggregationConfig. However, kDefault defers the actual aggregation choice to the instrument’s default (GetDefaultAggregationType returns kHistogram for histogram instruments). Existing callers can legitimately pass a histogram config while leaving the aggregation type at kDefault to customize bucket boundaries without overriding the default type; this change will now throw std::invalid_argument and prevent those views from being constructed. The check should accept histogram (and other concrete) config types when aggregation_type_ is kDefault because the concrete aggregation is resolved later based on the instrument descriptor.

Useful? React with 👍 / 👎.

The check is moved to ViweRegistry::AddView, to the deferred handling of kDefault logic is copied there. This is possible because AddView has both AggregationConfig and InstrumentationSelector.

codecov · 2025-10-31T01:06:52Z

Codecov Report

❌ Patch coverage is 65.51724% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.90%. Comparing base (6dcf647) to head (b61e528).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ude/opentelemetry/sdk/metrics/view/view_registry.h	68.00%	8 Missing ⚠️
...metry/sdk/metrics/aggregation/aggregation_config.h	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3732      +/-   ##
==========================================
- Coverage   90.01%   89.90%   -0.11%     
==========================================
  Files         225      225              
  Lines        7105     7134      +29     
==========================================
+ Hits         6395     6413      +18     
- Misses        710      721      +11

Files with missing lines	Coverage Δ
...metry/sdk/metrics/aggregation/aggregation_config.h	`89.48% <50.00%> (-10.52%)`	⬇️
...ude/opentelemetry/sdk/metrics/view/view_registry.h	`82.76% <68.00%> (-11.18%)`	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

marcalff · 2025-10-31T10:04:28Z

        attributes_processor_{std::move(attributes_processor)}
-  {}
+  {
+    // Validate that aggregation_type and aggregation_config match


Now that aggregation_config contains its proper type, can the aggregation_type parameter be removed instead ?

Probably not for now because the common AggregationConfig can still handle multiple aggregation types, like kDefault, kSum, etc. It is not 1:1 mapping.

marcalff

See some simplification.

Reneg973 · 2025-10-31T11:49:09Z

I guess also here exceptions are not allowed.
You could also add a noexcept where it's missing.

ThomsonTan · 2025-10-31T22:01:20Z

I guess also here exceptions are not allowed. You could also add a noexcept where it's missing.

@Reneg973 , where do add the noexcept, the View constructor?

Reneg973 · 2025-11-01T11:22:01Z

I guess also here exceptions are not allowed. You could also add a noexcept where it's missing.

@Reneg973 , where do add the noexcept, the View constructor?

On every function, also constructor, like this:

virtual AggregationType GetType() const noexcept { return AggregationType::kDefault; }

marcalff · 2025-11-03T20:45:22Z

+#if defined(__cpp_exceptions)
+        throw std::invalid_argument("AggregationType and AggregationConfig type mismatch");
+#else
+        std::abort();


So, someone defines a wrong view, and the process dies ?

If exception is not allowed, currently yes, the process dies.

lalitb · 2025-11-07T18:31:50Z

+
+        default:
+          // Unreachable: all AggregationType enum values are handled above
+          std::abort();


nit - this can be a utility method say IsDefaultAggregation(agg_type, inst_type, is_monotinic) in default_aggregation.h, but can be done afterwards if required at other places.

marcalff

For robustness, ignore invalid views / instruments / aggregations / etc.

A broken aggregation definition should not kill the application.

marcalff · 2025-11-08T17:05:54Z

+    {
+#if defined(__cpp_exceptions)
+      throw std::invalid_argument("instrument_selector, meter_selector, and view cannot be null");
+#else
+      std::abort();
+#endif
+    }


For every other failures in general, like illegal instrument names, the code just logs an error and ignores the instrument / view.

We should not throw exceptions or worse kill the process here, only ignore invalid configuration.

Replaced throw/abort with internal error log and ignores it.

marcalff

LGTM, see minor cleanup.

marcalff · 2025-11-14T11:06:47Z

@codex review

chatgpt-codex-connector · 2025-11-14T11:06:53Z

To use Codex here, create a Codex account and connect to github.

marcalff

LGTM.

marcalff · 2025-11-14T22:38:59Z

@ThomsonTan

The CI failure in install tests (conan) is due to a file system full on the github runner.

See:

Ubuntu image runs out of disk space actions/runner-images#13189

My take is that we still should merge this PR, the same failure exists in main already.

ThomsonTan added 2 commits October 30, 2025 17:17

Add tag to AggregationConfig for type checking

a2c48c6

Add test

d2330d1

ThomsonTan requested a review from a team as a code owner October 31, 2025 00:50

chatgpt-codex-connector Bot reviewed Oct 31, 2025

View reviewed changes

lalitb reviewed Oct 31, 2025

View reviewed changes

Comment thread sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h Outdated

Fix format

177d85a

ThomsonTan changed the title ~~Add tag to AggregationConfig for aggregation type validation~~ [Metrics] Add tag to AggregationConfig for aggregation type validation Oct 31, 2025

Address feedback

db221d8

marcalff reviewed Oct 31, 2025

View reviewed changes

Merge branch 'main' into add_tag_to_aggregationconfig

7ec33ec

ThomsonTan and others added 3 commits November 3, 2025 11:14

Merge branch 'main' into add_tag_to_aggregationconfig

4d9331c

Support build without exception

5dfc194

Remove logging

2c4266b

marcalff reviewed Nov 3, 2025

View reviewed changes

ThomsonTan and others added 7 commits November 3, 2025 15:59

Handle unittest in noexception build

6f04f06

Fix EXPECT_NO_THROW for no-exception build

363eb46

Fix iwyu

b500f61

Move check to AddView

af73f3a

Merge branch 'main' into add_tag_to_aggregationconfig

f28235c

Handle kDefault in AddView

6179d6f

Fix meter provider test

d3b4c42

ThomsonTan mentioned this pull request Nov 5, 2025

Create a AggregationConfig class for each AggregationType #3737

Closed

ThomsonTan and others added 3 commits November 5, 2025 10:27

Merge branch 'main' into add_tag_to_aggregationconfig

8e2a1b0

Fix iwyu

9dd5eae

Remove noexcept on AddView

36ef49a

ThomsonTan and others added 2 commits November 5, 2025 17:50

Add changelog

5e96531

Update changelog

f30cea6

ThomsonTan added the pr:please-review This PR is ready for review label Nov 6, 2025

ThomsonTan added 2 commits November 6, 2025 11:12

Merge branch 'main' into add_tag_to_aggregationconfig

f150c4a

Merge branch 'main' into add_tag_to_aggregationconfig

4b27470

lalitb approved these changes Nov 7, 2025

View reviewed changes

Merge branch 'main' into add_tag_to_aggregationconfig

4400651

marcalff requested changes Nov 8, 2025

View reviewed changes

kingpope0120 mentioned this pull request Nov 10, 2025

**<sub><sub>![P1 Badge](https://img.shields.io/badge/P1-orange?style=flat)</sub></sub> Allow histogram configs when aggregation type is default** #3743

Closed

ThomsonTan and others added 3 commits November 10, 2025 10:35

Merge branch 'main' into add_tag_to_aggregationconfig

a530053

Log View validation error and ignore it

01889b2

Add back noexcept on AddView

3867d1d

ThomsonTan changed the title ~~[Metrics] Add tag to AggregationConfig for aggregation type validation~~ [METRICS] Add tag to AggregationConfig for aggregation type validation Nov 10, 2025

Add noexcept back to AddView implementation

c1c5d0a

marcalff reviewed Nov 14, 2025

View reviewed changes

Comment thread sdk/include/opentelemetry/sdk/metrics/view/view.h Outdated

marcalff reviewed Nov 14, 2025

View reviewed changes

Comment thread sdk/include/opentelemetry/sdk/metrics/view/view_registry.h Outdated

marcalff reviewed Nov 14, 2025

View reviewed changes

ThomsonTan and others added 2 commits November 14, 2025 13:16

Merge branch 'main' into add_tag_to_aggregationconfig

83f6394

Address feedback

b61e528

marcalff approved these changes Nov 14, 2025

View reviewed changes

marcalff mentioned this pull request Nov 14, 2025

[RELEASE] Prepare opentelemetry-cpp release 1.24.0 #3748

Closed

marcalff added ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) and removed pr:please-review This PR is ready for review labels Nov 15, 2025

marcalff merged commit ec1275c into open-telemetry:main Nov 15, 2025
75 of 77 checks passed

BrewTestBot mentioned this pull request Nov 21, 2025

opentelemetry-cpp 1.24.0 Homebrew/homebrew-core#255376

Merged

1 task

Conversation

ThomsonTan commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

ThomsonTan Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

marcalff Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

ThomsonTan Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

Reneg973 commented Oct 31, 2025

Uh oh!

ThomsonTan commented Oct 31, 2025

Uh oh!

Reneg973 commented Nov 1, 2025

Uh oh!

marcalff Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

ThomsonTan Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

lalitb Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

marcalff Nov 8, 2025

Choose a reason for hiding this comment

Uh oh!

ThomsonTan Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

marcalff commented Nov 14, 2025

Uh oh!

chatgpt-codex-connector Bot commented Nov 14, 2025

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

marcalff commented Nov 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ThomsonTan commented Oct 31, 2025 •

edited

Loading

ThomsonTan Nov 6, 2025 •

edited

Loading

codecov Bot commented Oct 31, 2025 •

edited

Loading