Add logging integrations with logging and structlog by basepi · Pull Request #586 · elastic/apm-agent-python

basepi · 2019-09-09T20:47:25Z

Added new entries to the public API as helpers to retrieve transaction, trace, and span IDs
Added new ways to add APM tracing fields to logs:
- logging filter
- structlog processor
- automatic log_record_factory for python 3.2+
Added new "Logging Integrations" documentation
Added documentation around ingest for log correlation
Added tests for the filter, processor, and log_record_factory

Resolves #520

axw

Looking good!

beniwohli

Just a few nitpicks :) other than that, great stuff! We probably need to iterate on the docs a bit, it's going to be a fine line between being useful enough and not replicating a lot of logging and logstash/ingest docs :D

basepi · 2019-09-10T22:33:47Z

I still want to add a LogRecord factory for Python 3's setLogRecordFactory -- it will be enabled by default but you can turn it off via a config value.

I also still need to do some more documentation work around json logging, logstash grok, etc, for getting these new fields into ES in a useful way.

Otherwise I've implemented the other review comments, and will do the above ^ tomorrow.

beniwohli

Great work! Missed a couple things in my first review, sorry about that!

Plus add a test

basepi · 2019-09-13T02:29:09Z

Unless I forgot something major, I think this PR is ready for final review.

beniwohli

Great job on the documentation, I'll add @bmorelli25 to the reviewers to have a look at them too

Other than a few small things, I think this can be merged! Awesome job!

bmorelli25

Great work @basepi! I have some comments below. I'd recommend taking a look at #586 (comment) first. Let me know if you have any questions!

Also, just in case you were unaware, each PR generates a doc build that's available to easily see documentation changes: http://apm-agent-python_586.docs-preview.app.elstc.co/diff. There is a bug where the diff is duplicated, so only focus on the first links.

bmorelli25 · 2019-09-13T12:12:00Z

I just had another thought about this PR. I'm currently working on documentation for the integration between the APM UI and the Logs UI. The idea for those docs was to include the Log correlation instructions in a central location. You can see the draft I have here: https://github.com/elastic/apm-server/pull/2672/files#diff-e5d2431c1637befb5513a06f10499bfb. Right now I haven't put any work into it. It's just copy/pasted from the logs documentation, and probably doesn't make any sense in this context.

I'm wondering what your thoughts would be on the log correlation documentation living there vs in the Agent repos. It looks like for structured logs you use a logging package specific to python, but for unstructured the solution is more flexible and might apply to other agents as well? Please correct me if I'm wrong. Would like @beniwohli's thoughts as well.

beniwohli · 2019-09-13T13:42:10Z

@bmorelli25 I think everything starting from https://github.com/elastic/apm-agent-python/pull/586/files#diff-b9aa18d4f6625af66a2ba37d4263e914R118 could live in a central documentation, with the assumption that all other agents can replicate the format that @basepi proposed.

basepi · 2019-09-13T14:17:37Z

Yeah similar documentation could work for other languages. Obviously getting the unstructured log lines to the parseable state will differ based on the language/logging library the user is working with. If we ended up centralizing it, I'd just replace ours with a link to the central docs.

bmorelli25 · 2019-09-13T14:20:02Z

Great, thanks. Let's stick with this format for now and I'll update if/when we get to that point.

Co-Authored-By: Benjamin Wohlwend <[email protected]>

Co-Authored-By: Brandon Morelli <[email protected]>

Also fix up some imports and assert elasticapm_labels everywhere

Because of the dynamic insertion in Client, this test was failing on the wider integration tests (perhaps because it had been inserted on previous test runs? I'm actually not sure). Anyway, we test that it works with an instantiated Client so I think this test is unnecessary.

basepi · 2019-09-16T23:47:01Z

Alright, I've incorporated all feedback and have the tests back to green. This is ready for final approval.

bmorelli25

Nice work @basepi! Other than Andrew's recommendation above, docs LGTM!

beniwohli

Just a few minor documentation bits. Feel free to merge after that!

Co-Authored-By: Benjamin Wohlwend <[email protected]> Co-Authored-By: Andrew Wilkins <[email protected]>

…ging520

Add logging integrations with logging and structlog

2b3d017

basepi requested a review from beniwohli September 9, 2019 20:47

Update changelog

e757f82

axw reviewed Sep 10, 2019

View reviewed changes

Comment thread docs/logging.asciidoc

Comment thread elasticapm/traces.py Outdated

beniwohli suggested changes Sep 10, 2019

View reviewed changes

Comment thread docs/logging.asciidoc

Comment thread docs/logging.asciidoc

Comment thread elasticapm/handlers/logging.py Outdated

Comment thread elasticapm/handlers/logging.py Outdated

basepi added 3 commits September 10, 2019 16:13

Simplify and make naming consistent

996f46a

Add span.id to logging (and pub API) as well

bbf75db

Merge remote-tracking branch 'upstream/master' into logging520

c4966c7

beniwohli suggested changes Sep 11, 2019

View reviewed changes

Comment thread docs/api.asciidoc Outdated

Comment thread elasticapm/handlers/logging.py Outdated

Comment thread elasticapm/handlers/logging.py Outdated

Comment thread tests/handlers/logging/logging_tests.py Outdated

basepi added 7 commits September 11, 2019 16:15

Fix more refactor leftovers, test against actual ID

b7203cd

Add the tracing fields under "correct" name as well

d1e948b

Add log_record_factory and DRY some code

4e505f4

Merge remote-tracking branch 'upstream/master' into logging520

2c1c0d8

Automatically install the log_record_factory

5ce4916

Plus add a test

Update changelog

648203f

Add more documentation about correlation

6e5f4e1

beniwohli suggested changes Sep 13, 2019

View reviewed changes

Comment thread docs/logging.asciidoc Outdated

Comment thread docs/logging.asciidoc

Comment thread docs/logging.asciidoc Outdated

Comment thread elasticapm/base.py Outdated

Comment thread elasticapm/base.py Outdated

Comment thread elasticapm/handlers/logging.py Outdated

beniwohli requested a review from bmorelli25 September 13, 2019 08:07

bmorelli25 requested changes Sep 13, 2019

View reviewed changes

bmorelli25 reviewed Sep 13, 2019

View reviewed changes

Comment thread docs/logging.asciidoc Outdated

bmorelli25 reviewed Sep 13, 2019

View reviewed changes

Comment thread docs/logging.asciidoc Outdated

basepi self-assigned this Sep 13, 2019

basepi added the [zube]: Inbox label Sep 13, 2019

basepi removed their assignment Sep 13, 2019

basepi and others added 7 commits September 13, 2019 08:20

s/elasticapm/The Elastic APM agent/

e715f36

Co-Authored-By: Benjamin Wohlwend <[email protected]>

Fix project-level import

b18ebb1

Co-Authored-By: Benjamin Wohlwend <[email protected]>

Apply suggestions from code review

a7defdf

Co-Authored-By: Brandon Morelli <[email protected]>

Fix up tracing fields link

27ec7c9

Apply suggestions from code review

e2050c1

Co-Authored-By: Brandon Morelli <[email protected]>

Separate log_record_factory tests from client instantiation

5ffae42

Also fix up some imports and assert elasticapm_labels everywhere

More docs changes from reviews

6a1a2cb

basepi commented Sep 13, 2019

View reviewed changes

Comment thread docs/logging.asciidoc Outdated

basepi added 5 commits September 13, 2019 18:20

Fix another accidental relative import

94fc403

Use full link for ecs docs

a58b7bc

Fix for py2 testing

3d9d599

Fix missing import (inline due to circular import)

92c7a87

basepi requested review from beniwohli and bmorelli25 September 16, 2019 23:46

axw reviewed Sep 17, 2019

View reviewed changes

Comment thread docs/logging.asciidoc Outdated

bmorelli25 approved these changes Sep 17, 2019

View reviewed changes

beniwohli approved these changes Sep 17, 2019

View reviewed changes

Comment thread docs/api.asciidoc Outdated

Comment thread docs/configuration.asciidoc Outdated

Comment thread docs/logging.asciidoc Outdated

basepi and others added 3 commits September 17, 2019 07:57

Apply suggestions from code review

6b68a7f

Co-Authored-By: Benjamin Wohlwend <[email protected]> Co-Authored-By: Andrew Wilkins <[email protected]>

Merge branch 'master' of github.com:elastic/apm-agent-python into log…

a152855

…ging520

Implement config doc changes

f2450da

beniwohli mentioned this pull request Sep 17, 2019

use structlog for logging elastic/opbeans-python#29

Merged

Fix structlog processor to skip None values

4a065ac

bmorelli25 mentioned this pull request Sep 17, 2019

[docs] APM / Log UI integration documentaiton elastic/apm-server#2672

Merged

basepi merged commit 4b92589 into elastic:master Sep 17, 2019

zube Bot added [zube]: Done and removed [zube]: Inbox labels Sep 17, 2019

basepi mentioned this pull request Sep 18, 2019

Add logging Formatter class #595

Merged

Uh oh!

Conversation

basepi commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

beniwohli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

basepi commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beniwohli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

basepi commented Sep 13, 2019

Uh oh!

beniwohli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmorelli25 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmorelli25 commented Sep 13, 2019

Uh oh!

beniwohli commented Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

basepi commented Sep 13, 2019

Uh oh!

bmorelli25 commented Sep 13, 2019

Uh oh!

Uh oh!

basepi commented Sep 16, 2019

Uh oh!

Uh oh!

bmorelli25 left a comment

Choose a reason for hiding this comment

Uh oh!

beniwohli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

basepi commented Sep 9, 2019 •

edited

Loading

basepi commented Sep 10, 2019 •

edited

Loading

bmorelli25 left a comment •

edited

Loading

beniwohli commented Sep 13, 2019 •

edited

Loading