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

Logging: Defensively access provider resource #890

Merged

Conversation

owais
Copy link
Contributor

@owais owais commented Feb 1, 2022

Description

Now service name is extracted from the provider defensively and lazily.
This accounts for an SDK that does not provide access to "resource" via
TracerProviders and for lazy initialization of TracerProviders.

Fixes #810

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Added new test to cover use case
  • Manually

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@owais owais requested a review from a team February 1, 2022 13:06
@owais owais force-pushed the instrumentation-use-sdk-defensively branch 4 times, most recently from c5ea1f6 to 059eca9 Compare February 1, 2022 13:40
Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

What does it mean for something to be defensive?

@owais
Copy link
Contributor Author

owais commented Feb 1, 2022

I guess gracefully downgrade instead of crashing when some condition is not met like using getattr(.., .., None) instead of directly accessing an attribute or checking for the presence of something before using it. May be there is a better term for it out there I'm not aware of ? :)

@owais owais force-pushed the instrumentation-use-sdk-defensively branch from 059eca9 to 0a8c8f3 Compare February 1, 2022 18:31
old_factory = logging.getLogRecordFactory()
LoggingInstrumentor._old_factory = old_factory

service_name = None

def record_factory(*args, **kwargs):
record = old_factory(*args, **kwargs)

record.otelSpanID = "0"
record.otelTraceID = "0"
Copy link
Member

Choose a reason for hiding this comment

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

Probably out of scope, what do you think about setting service name to "" here similar to span, traceid and defensively access resource on the below span instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about accessing resource from span as span API does not define resource at all. ReadableSpan defines resource which should only be used by the SDK. I know somewhat similar argument applies to provider but for some reason I feel doing it with provider is a lesser evil. Happy to hear other ideas/counter-arguments but may be we shold open an issue and discuss there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually we should discuss it in the next SIG call. Not span vs provider but the fact that an instrumentation depends on an SDK feature which shouldn't be the case.

Copy link
Member

Choose a reason for hiding this comment

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

the fact that an instrumentation depends on an SDK feature which shouldn't be the case.

Isn't this an exception and I remember there was some discussion around if instrumentation-logging should actually be an instrumentation. I am not aware of any other instrumentation depending sdk feature.

@owais owais force-pushed the instrumentation-use-sdk-defensively branch from 7215334 to 91ec1a2 Compare February 1, 2022 19:52
Now service name is extracted from the provider defensively and lazily.
This accounts for an SDK that does not provide access to "resource" via
TracerProviders and for lazy initialization of TracerProviders.

Fixes open-telemetry#810
@owais owais force-pushed the instrumentation-use-sdk-defensively branch from dea305c to 6560157 Compare February 1, 2022 19:52
@ocelotl ocelotl merged commit 8d309af into open-telemetry:main Feb 3, 2022
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.

error with logging instrumentation - AttributeError: 'ProxyTracerProvider' object has no attribute 'resource'
4 participants