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

Implement events sdk #4176

Merged
merged 19 commits into from
Sep 10, 2024
Merged

Implement events sdk #4176

merged 19 commits into from
Sep 10, 2024

Conversation

lzchen
Copy link
Contributor

@lzchen lzchen commented Sep 4, 2024

From the specs

Fixes #4156

@lzchen lzchen requested a review from a team September 4, 2024 18:20
Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

LGTM, I think we are missing the entry point

Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

So looks like nobody is using / testing the logger provider entry point so probably no need to poke with that here either.

(All my commits are for fixing linting)

Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

Testing this locally and this is not working out of the box and am not able to figure out how it's supposed to load something that is not the default.

Anyway after adding to pyproject.toml:

[project.entry-points.opentelemetry_event_logger_provider]
sdk_event_logger_provider = "opentelemetry.sdk._events:EventLoggerProvider"

And using the env var to set it via:

OTEL_PYTHON_EVENT_LOGGER_PROVIDER=sdk_event_logger_provider

I get an exception because the proxy logger does not have the resource attribute:

  File "/venv/lib/python3.10/site-packages/opentelemetry/sdk/_events/__init__.py", line 59, in emit
    resource=self._logger.resource,
AttributeError: 'ProxyLogger' object has no attribute 'resource'

Adding OTEL_PYTHON_LOGGER_PROVIDER=sdk_logger_provider did fix the the exception but still did not see anything exported under opentelemetry-instrument. This could well be a generic issue with logs more than something events specific though (or a case of PEBKAC).

In the api pyproject we should probably add the following to match what the other providers are doing:

[project.entry-points.opentelemetry_event_logger_provider]
default_event_logger_provider = "opentelemetry._events:NoOpEventLoggerProvider"

Copy link
Member

@pmcollins pmcollins left a comment

Choose a reason for hiding this comment

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

LGTM overall -- thx for doing this. Added a comment about testing methodology.

opentelemetry-sdk/tests/events/test_events.py Show resolved Hide resolved
@lzchen
Copy link
Contributor Author

lzchen commented Sep 9, 2024

@xrmx

I would prefer to keep the scope of this pr to just manual implementation and include support for autoinstrumentation in a separate pr.

@emdneto
Copy link
Member

emdneto commented Sep 10, 2024

Thanks a lot for working on this one 🙌🏻 . For anyone who wants to review, here is my basic checklist:

  • The EventLoggerProvider MUST be implemented as a proxy to an instance of LoggerProvider.
  • In the case where an invalid name (null or empty string) is specified, a working EventLogger MUST be returned as a fallback rather than returning null or throwing an exception.
  • The EventLogger MUST be implemented as a proxy to an instance of Logger.
  • The EventLoggerProvider MUST accept an instance of LoggerProvider.
  • Emit a LogRecord representing an Event.
  • Implementation Requirements of emit the Event

Copy link
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

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

Also tested with a snippet.

@lzchen lzchen merged commit bd51fcb into open-telemetry:main Sep 10, 2024
376 checks passed
@lzchen lzchen deleted the event branch September 10, 2024 15:44
@xrmx xrmx mentioned this pull request Oct 21, 2024
10 tasks
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.

Implement Events SDK
5 participants