Skip to content

chore(configuration): remove obsolete attribute limits TODO#6891

Merged
trentm merged 1 commit into
open-telemetry:mainfrom
LarryHu0217:codex/resource-attribute-limits-6842
Jul 9, 2026
Merged

chore(configuration): remove obsolete attribute limits TODO#6891
trentm merged 1 commit into
open-telemetry:mainfrom
LarryHu0217:codex/resource-attribute-limits-6842

Conversation

@LarryHu0217

@LarryHu0217 LarryHu0217 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What changed

Remove the obsolete TODO(6842) comment from resource attribute merging. No resource attribute limit behavior is added.

Why

The OpenTelemetry specification says resource attributes should be exempt from the general attribute limits. Keeping the TODO implied behavior that should not be implemented, so this PR now removes only that misleading comment and leaves resource behavior unchanged.

Related to #6842.

Validation

  • Prettier check for experimental/packages/configuration/src/FileConfigFactory.ts
  • git diff --check

AI assistance disclosure

ChatGPT was used to assist with code exploration, implementation, and test preparation. I reviewed the changes and validation results.

@LarryHu0217
LarryHu0217 marked this pull request as ready for review July 7, 2026 21:25
@LarryHu0217
LarryHu0217 requested a review from a team as a code owner July 7, 2026 21:25
@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.09%. Comparing base (a79c4a4) to head (1856b7c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6891   +/-   ##
=======================================
  Coverage   95.08%   95.09%           
=======================================
  Files         394      394           
  Lines       13997    13997           
  Branches     3224     3224           
=======================================
+ Hits        13309    13310    +1     
+ Misses        688      687    -1     
Files with missing lines Coverage Δ
...al/packages/configuration/src/FileConfigFactory.ts 97.77% <ø> (ø)

... 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@maryliag

maryliag commented Jul 8, 2026

Copy link
Copy Markdown
Member

Hi @LarryHu0217 , thank you for your contribution!
I want to take this opportunity for a reminder of our OpenTelemetry Generative AI policy: disclose any AI use in your contribution, and communicate (PR descriptions, review replies) in your own words rather than AI-generated text.

I'll review this PR this week, in the meantime make sure you add a changelog.

Also, you added here and on a few other issues the phrase "No compensation requested.", this is an open source project, there is no compensation to be requested, so there is no need to comment that and also no expectation of receiving any monetary compensation :)

@LarryHu0217

LarryHu0217 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for letting me know. I added the changelog and cleaned up the PR description.

@LarryHu0217
LarryHu0217 force-pushed the codex/resource-attribute-limits-6842 branch from 570b62b to 4700903 Compare July 8, 2026 16:14

@trentm trentm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@LarryHu0217 I feel a little bad. I'm the one that added that "TODO" comment for this as part of the work in #6765.

However, from https://opentelemetry.io/docs/specs/otel/common/#exempt-entities in the spec:

Resource attributes SHOULD be exempt from the limits described above ...

So I think we should not make this change. We should remove the "// TODO(6842): handle attribute limits, if any" comment and close #6842 as wontfix.

@LarryHu0217
LarryHu0217 force-pushed the codex/resource-attribute-limits-6842 branch from 4415264 to 1856b7c Compare July 9, 2026 22:27
@LarryHu0217 LarryHu0217 changed the title fix(configuration): apply resource attribute limits chore(configuration): remove obsolete attribute limits TODO Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for catching this. I updated the PR to remove only the obsolete TODO and dropped the attribute-limit implementation and related tests. Resource behavior is now unchanged.

Copy link
Copy Markdown
Contributor Author

Could a maintainer add the Skip Changelog label for this comment-only cleanup? The changelog check requires it, and I do not have permission to apply repository labels. The lint job also failed on an unrelated external link in doc/semconv-stable-http-and-database.md; the one changed file passed formatting and git diff --check.

@trentm

trentm commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Lint failure is unrelated. Link-checking is DoS'ing the opentelemetry.io site.

@trentm

trentm commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Thanks for the quick turn around.

@trentm
trentm added this pull request to the merge queue Jul 9, 2026
Merged via the queue into open-telemetry:main with commit 17e4f8c Jul 9, 2026
28 of 30 checks passed
@otelbot

otelbot Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Thank you for your contribution @LarryHu0217! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants