Add support for user defined attributes in OTLPHandler #1952

adriangb · 2021-07-14T04:13:13Z

Description

This gives users the ability to inject custom attributes into emitted logs.

I think this should solve the discussion in https://github.com/open-telemetry/opentelemetry-python/pull/1903/files#r653092699

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?

I added 2 tests for the functionality verifying that the attributes end up in the emitted LogRecord and that the user can specify a sub-key to pull attributes from.

Does This PR Require a Contrib Repo Change?

Yes. - Link to PR:
No.

Checklist:

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

linux-foundation-easycla · 2021-07-14T04:13:16Z

The committers are authorized under a signed CLA.

✅ Adrian Garcia Badaracco (4c6d5e9, 28632ca, 93e0037)

opentelemetry-sdk/src/opentelemetry/sdk/logs/__init__.py

srikanthccv

Thanks for trying to complete todo item. This was originally added because there is some clarification I wanted to get from logs sig and some questions on semantic conventions. I am not sure if we want to proceed with this as is.

opentelemetry-sdk/src/opentelemetry/sdk/logs/__init__.py

codeboten · 2021-08-03T16:25:18Z

@lonewolf3739 any update from the SIG on this one? Just wanted to make sure we can get this reviewed if it's still useful.

srikanthccv · 2021-08-03T16:48:56Z

This is fine. There were other changes when this PR opened and now it just has the extra attributes support.

srikanthccv · 2021-08-03T16:50:32Z

opentelemetry-sdk/src/opentelemetry/sdk/logs/__init__.py

@@ -244,20 +244,58 @@ def force_flush(self, timeout_millis: int = 30000) -> bool:
        return True


+# skip natural LogRecord attributes
+# http://docs.python.org/library/logging.html#logrecord-attributes
+_RESERVED_ATTRS = frozenset(


We are skipping now to separate the user-defined extra attributes but we will revisit this later if needed.

adriangb · 2021-08-04T15:21:47Z

@ocelotl anything needed on my end to merge this?

srikanthccv · 2021-08-04T15:51:22Z

@adriangb Please fix the lint.

ocelotl · 2021-08-04T16:18:16Z

@ocelotl anything needed on my end to merge this?

Yes, as @lonewolf3739 mentioned, please make sure the lint tests are passing. This might help.

adriangb · 2021-08-04T16:23:55Z

I'm working on it. Your setup / instructions in CONTRIBUTING.md are very broken (and this has been the case for months, i.e. #1846), so something that should be happening automatically / take 1 minute is taking a while. Please fix that, otherwise contributing is very hard.

adriangb · 2021-08-04T16:34:21Z

Well using your link I tried tox -e lint (and after cloning the contrib repo) I get:

tox -e lint                                                                                          
lint run-test-pre: commands[2] | python -m pip install .../opentelemetry-python/opentelemetry-python-contrib/opentelemetry-instrumentation
ERROR: Invalid requirement: '.../opentelemetry-python/opentelemetry-python-contrib/opentelemetry-instrumentation'
Hint: It looks like a path. File '.../opentelemetry-python/opentelemetry-python-contrib/opentelemetry-instrumentation' does not exist.

Looking at the contrib-repo, I can see that there is no opentelemetry-instrumentation folder. Am I missing something here or is tox -e lint also broken?

I'm also pretty confused as to why I need stuff from contrib to test (never mind lint) stuff in core. What is the point of having a core package if it depends on the contrib package 😕 ?

ocelotl · 2021-08-04T16:36:11Z

Well using your link I tried tox -e lint (and after cloning the contrib repo) I get:
tox -e lint                                                                                          
lint run-test-pre: commands[2] | python -m pip install .../opentelemetry-python/opentelemetry-python-contrib/opentelemetry-instrumentation
ERROR: Invalid requirement: '.../opentelemetry-python/opentelemetry-python-contrib/opentelemetry-instrumentation'
Hint: It looks like a path. File '.../opentelemetry-python/opentelemetry-python-contrib/opentelemetry-instrumentation' does not exist.
Looking at the contrib-repo, I can see that there is no opentelemetry-instrumentation folder. Am I missing something here or is tox -e lint also broken?

Please make sure your branch is updated against main. We moved opentelemetry-instrumentation to the core repo several commits ago.

codeboten · 2021-08-04T16:36:46Z

@adriangb the opentelemetry-instrumentation folder was moved from contrib to this repo. Is it possible the tox file is out of sync w/ main?

adriangb · 2021-08-04T16:44:20Z

Well I just went into the logs and fixed the 1 line pylint was complaining about.

adriangb · 2021-08-04T16:45:20Z

To avoid this #1952 (comment) in the future, I'd suggest you move away from long-lived feature branches. They're problematic for many reasons.

codeboten · 2021-08-04T16:46:54Z

To avoid this #1952 (comment) in the future, I'd suggest you move away from long-lived feature branches. They're problematic for many reasons.

I agree, the logs/metrics branches are a special case for the work around experimental features, but yes, long lived branches are terrible

adriangb · 2021-08-04T17:32:31Z

Looks like we're all ✅ now @ocelotl

ocelotl · 2021-08-04T19:41:53Z

Let's see...

ocelotl · 2021-08-04T19:45:15Z

I see tests that are still running will give this a bit more time

adriangb · 2021-08-04T19:46:42Z

Are those tests really running or is CI broken / stuck? It's been "running" for almost 2 hours with no output.

Edit: it looks like at least contrib-build (py37, instrumentation, ubuntu-latest) is not even running tests. It's stuck on Install tox. Installing tox cannot take 2 hours.

adriangb · 2021-08-04T21:06:59Z

@ocelotl can you please restart jobs here too?

ocelotl · 2021-08-04T21:07:48Z

Sure

lzchen · 2021-08-04T21:56:08Z

opentelemetry-sdk/src/opentelemetry/sdk/logs/__init__.py

+    @staticmethod
+    def _get_attributes(record: logging.LogRecord) -> Attributes:
+        return {
+            k: v for k, v in vars(record).items() if k not in _RESERVED_ATTRS


Is extra the only keyword that will be filtered in?

Yes, that's how the stdlib logging library works.

Documentation: https://docs.python.org/3/library/logging.html#logging.Logger.debug (paragraph 2)

Source: https://github.com/python/cpython/blob/80f33f266b4ad5925a3e58ea3a54ae139a6b6f0e/Lib/logging/__init__.py#L1597-L1601

adriangb · 2021-08-04T22:06:51Z

@ocelotl doesn't look like CI jobs were restarted, I'm still seeing the same Installing tox stage that's now at 4 hours.

ocelotl · 2021-08-04T22:32:19Z

@ocelotl doesn't look like CI jobs were restarted, I'm still seeing the same Installing tox stage that's now at 4 hours.

Hmm... ok, seems like they have actually restarted now. Sorry for all the inconveniences, apparently the entire Github opentelemetry organization is having issues with CI.

adriangb · 2021-08-05T01:43:48Z

Looks like we're passing aside from a benchmark that says something is 7x slower (?). I don't think this PR would make anything 7x slower, so I'm guessing it's more strange CI stuff?

ocelotl · 2021-08-05T04:19:00Z

Looks like we're passing aside from a benchmark that says something is 7x slower (?). I don't think this PR would make anything 7x slower, so I'm guessing it's more strange CI stuff?

Yes, seems like more trouble with Github-CI. The only thing I can do now is try running them again 🤷

I have also filed a ticket to Github regarding this problem.

srikanthccv · 2021-08-12T18:45:07Z

@codeboten this has different base branch. Same fix should applied to it as well.

codeboten · 2021-08-12T21:11:32Z

@codeboten this has different base branch. Same fix should applied to it as well.

@lonewolf3739 correct, I created a PR here #2037

ocelotl · 2021-08-13T20:08:33Z

Re-running stuck jobs for this PR ✌️

adriangb · 2021-08-14T07:04:43Z

Re-running stuck jobs for this PR ✌️

Thank you! Looks like everything is passing now except for benchmarks, which I'm guessing is noise? All we're adding there is a single dict-comprehension. Except in some extreme case (all that happens in the entire program is logging and each log has 1000 user defined attributes), this should not change performance at all.

ocelotl · 2021-08-16T12:03:43Z

Re-running stuck jobs for this PR v

Thank you! Looks like everything is passing now except for benchmarks, which I'm guessing is noise? All we're adding there is a single dict-comprehension. Except in some extreme case (all that happens in the entire program is logging and each log has 1000 user defined attributes), this should not change performance at all.

We have seen performance issues with Pypy before, I'll try again.

If it still refuses to pass we may have to take a look into the performance requirements for Pypy, it should be ok that they are lower than the rest.

…y#1952)

* Add initial overall structure and classes for logs sdk (#1894) * Add global LogEmitterProvider and convenience function get_log_emitter (#1901) * Add OTLPHandler for standard library logging module (#1903) * Add LogProcessors implementation (#1916) * Fix typos in test_handler.py (#1953) * Add support for OTLP Log exporter (#1943) * Add support for user defined attributes in OTLPHandler (#1952) * use timeout in force_flush (#2118) * use timeout in force_flush * fix lint * Update opentelemetry-sdk/src/opentelemetry/sdk/logs/export/__init__.py Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com> * fix lint Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com> * add a ConsoleExporter for logging (#2099) Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com> * Update SDK docs and Add example with OTEL collector logging (debug) exporter (#2050) * Fix exception in severity number transformation (#2208) * Fix exception with warning message transformation * Fix lint * Fix lint * fstring * Demonstrate how to set the Resource for LogEmitterProvider (#2209) * Demonstrate how to set the Resource for LogEmitterProvider Added a Resource to the logs example to make it more complete. Previously it was using the built-in Resource. Now it adds the service.name and service.instance.id attributes. The resulting emitted log records look like this: ``` Resource labels: -> telemetry.sdk.language: STRING(python) -> telemetry.sdk.name: STRING(opentelemetry) -> telemetry.sdk.version: STRING(1.5.0) -> service.name: STRING(shoppingcart) -> service.instance.id: STRING(instance-12) InstrumentationLibraryLogs #0 InstrumentationLibrary __main__ 0.1 LogRecord #0 Timestamp: 2021-10-14 18:33:43.425820928 +0000 UTC Severity: ERROR ShortName: Body: Hyderabad, we have a major problem. Trace ID: ce1577e4a703f42d569e72593ad71888 Span ID: f8908ac4258ceff6 Flags: 1 ``` * Fix linting * Use batch processor in example (#2225) * move logs to _logs (#2240) * move logs to _logs * fix lint * move log_exporter to _log_exporter as it's still experimental (#2252) Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com> Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Co-authored-by: Leighton Chen <lechen@microsoft.com> Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com> Co-authored-by: Owais Lone <owais@users.noreply.github.com>

…y#1952)

adriangb added 2 commits July 13, 2021 23:07

Add support for custom attributes in OTLPHandler

4c6d5e9

blacken

28632ca

adriangb requested a review from a team as a code owner July 14, 2021 04:13

adriangb requested review from ocelotl and srikanthccv and removed request for a team July 14, 2021 04:13

adriangb added 3 commits July 13, 2021 23:38

Update CHANGELOG.md

93e0037

Dummy change for CLA

db57083

fix typo

826b02a

adriangb commented Jul 14, 2021

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/logs/__init__.py Outdated Show resolved Hide resolved

srikanthccv reviewed Jul 14, 2021

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/logs/__init__.py Outdated Show resolved Hide resolved

opentelemetry-sdk/src/opentelemetry/sdk/logs/__init__.py Show resolved Hide resolved

adriangb added 6 commits July 14, 2021 10:41

Remove atribute_keyu param

231fb16

Use suggested attributes

ea248c2

Fix parens

dd2b3dc

Add example test

5765374

fmt

248ac56

remove example test

f8f4876

Merge branch 'logs' into custom-attributes-log-handler

e0d3b5e

srikanthccv approved these changes Aug 3, 2021

View reviewed changes

ocelotl approved these changes Aug 4, 2021

View reviewed changes

make _get_attributes a static method

539df29

lzchen reviewed Aug 4, 2021

View reviewed changes

lzchen approved these changes Aug 10, 2021

View reviewed changes

codeboten approved these changes Aug 12, 2021

View reviewed changes

Merge branch 'logs' into custom-attributes-log-handler

2a4ed23

lzchen merged commit d0dbc0b into open-telemetry:logs Aug 16, 2021

adriangb mentioned this pull request Sep 1, 2021

Falcon 3 support open-telemetry/opentelemetry-python-contrib#644

Merged

lzchen pushed a commit to lzchen/opentelemetry-python that referenced this pull request Oct 15, 2021

Add support for user defined attributes in OTLPHandler (open-telemetr…

8ef53c0

…y#1952)

codeboten pushed a commit that referenced this pull request Oct 29, 2021

Add support for user defined attributes in OTLPHandler (#1952)

acfd22e

codeboten pushed a commit to codeboten/opentelemetry-python that referenced this pull request Nov 3, 2021

Add support for user defined attributes in OTLPHandler (open-telemetr…

ca4d91c

…y#1952)

lzchen pushed a commit that referenced this pull request Nov 4, 2021

Add support for user defined attributes in OTLPHandler (#1952)

914ee51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for user defined attributes in OTLPHandler #1952

Add support for user defined attributes in OTLPHandler #1952

adriangb commented Jul 14, 2021 •

edited

Loading

linux-foundation-easycla bot commented Jul 14, 2021 •

edited

Loading

srikanthccv left a comment

codeboten commented Aug 3, 2021

srikanthccv commented Aug 3, 2021

srikanthccv Aug 3, 2021

adriangb commented Aug 4, 2021

srikanthccv commented Aug 4, 2021

ocelotl commented Aug 4, 2021

adriangb commented Aug 4, 2021 •

edited

Loading

adriangb commented Aug 4, 2021 •

edited

Loading

ocelotl commented Aug 4, 2021

codeboten commented Aug 4, 2021

adriangb commented Aug 4, 2021

adriangb commented Aug 4, 2021

codeboten commented Aug 4, 2021

adriangb commented Aug 4, 2021

ocelotl commented Aug 4, 2021

ocelotl commented Aug 4, 2021

adriangb commented Aug 4, 2021 •

edited

Loading

adriangb commented Aug 4, 2021

ocelotl commented Aug 4, 2021

lzchen Aug 4, 2021

adriangb Aug 4, 2021

adriangb commented Aug 4, 2021

ocelotl commented Aug 4, 2021

adriangb commented Aug 5, 2021

ocelotl commented Aug 5, 2021

srikanthccv commented Aug 12, 2021

codeboten commented Aug 12, 2021

ocelotl commented Aug 13, 2021

adriangb commented Aug 14, 2021

ocelotl commented Aug 16, 2021

Add support for user defined attributes in OTLPHandler #1952

Add support for user defined attributes in OTLPHandler #1952

Conversation

adriangb commented Jul 14, 2021 • edited Loading

Description

Type of change

How Has This Been Tested?

Does This PR Require a Contrib Repo Change?

Checklist:

linux-foundation-easycla bot commented Jul 14, 2021 • edited Loading

srikanthccv left a comment

Choose a reason for hiding this comment

codeboten commented Aug 3, 2021

srikanthccv commented Aug 3, 2021

srikanthccv Aug 3, 2021

Choose a reason for hiding this comment

adriangb commented Aug 4, 2021

srikanthccv commented Aug 4, 2021

ocelotl commented Aug 4, 2021

adriangb commented Aug 4, 2021 • edited Loading

adriangb commented Aug 4, 2021 • edited Loading

ocelotl commented Aug 4, 2021

codeboten commented Aug 4, 2021

adriangb commented Aug 4, 2021

adriangb commented Aug 4, 2021

codeboten commented Aug 4, 2021

adriangb commented Aug 4, 2021

ocelotl commented Aug 4, 2021

ocelotl commented Aug 4, 2021

adriangb commented Aug 4, 2021 • edited Loading

adriangb commented Aug 4, 2021

ocelotl commented Aug 4, 2021

lzchen Aug 4, 2021

Choose a reason for hiding this comment

adriangb Aug 4, 2021

Choose a reason for hiding this comment

adriangb commented Aug 4, 2021

ocelotl commented Aug 4, 2021

adriangb commented Aug 5, 2021

ocelotl commented Aug 5, 2021

srikanthccv commented Aug 12, 2021

codeboten commented Aug 12, 2021

ocelotl commented Aug 13, 2021

adriangb commented Aug 14, 2021

ocelotl commented Aug 16, 2021

adriangb commented Jul 14, 2021 •

edited

Loading

linux-foundation-easycla bot commented Jul 14, 2021 •

edited

Loading

adriangb commented Aug 4, 2021 •

edited

Loading

adriangb commented Aug 4, 2021 •

edited

Loading

adriangb commented Aug 4, 2021 •

edited

Loading