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

Resource: remove unnecessary diagnostic when accessing attributes #5119

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

seemk
Copy link
Contributor

@seemk seemk commented Nov 7, 2024

Discussion issue: #5114

Since Resource already has 2 methods to deal with yet to be resolved attributes (asyncAttributesPending and waitForAsyncAttributes), the diagnostic seems redundant and perhaps misleading (since it's more of a debug message than an actual error).

@seemk seemk requested a review from a team as a code owner November 7, 2024 09:53
Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.21%. Comparing base (f1ef596) to head (5afc5f4).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5119      +/-   ##
==========================================
- Coverage   93.21%   93.21%   -0.01%     
==========================================
  Files         315      315              
  Lines        8096     8094       -2     
  Branches     1622     1621       -1     
==========================================
- Hits         7547     7545       -2     
  Misses        549      549              
Files with missing lines Coverage Δ
packages/opentelemetry-resources/src/Resource.ts 100.00% <ø> (ø)

@pichlermarc
Copy link
Member

Hi, thanks for reaching out - I think the reason why this pops up is because there's some other bug somewhere that triggers this message. IIRC the reason why this was put there was the following (some of the decisions made there predate my time on the project, but I'll try to give a summary as I understand it):

ResourceDetector.detect() was (still is) async. The reason for that was that there were some resource detectors that needed to perform an async operation to retrieve the resource. That was not great as the users had to await resource detection, which most of the time is not feasible as it would delay SDK registration and would cause dropped telemetry. At some point, we then introduced ResourceDetectorSync that would create resources that had async resolvable Attributes, a new feature at the time.

At the same time that this feature was introduced, awaiting the Attributes was moved to the export pipeline (SpanProcessor, MetricReader, ...) - that's the call to IResource#waitForAsyncAttributes() that's always called before before accessing IResource.attributes. If waitForAsyncAttributes() is not awaited, before accessing attributes this error is supposed to be logged - which is happening here. The error is supposed to tell the user that the attributes returned will be incomplete.

What that means is (from my perspective) is that one of these two statements should be true:

  • there is somewhere a call that accesses the attributes in such a way, which is incorrect (in the SDK setup - like NodeSDK for instance, the export pipeline, or the user code)
    • if that's the case we need to address it there
  • there is a bug in @opentelemetry/resource somewhere that needs to be addressed because it logs that error when it actually should not
    • if that's the case we need to come up with a test-case that triggers this an error message when there shouldn't be one and then address the bug

So to summarize: I'm worried we may be hiding a deeper issue here by removing that log. I think that we have to instead try to come up with a solution to the underlying problem.

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.

2 participants