Skip to content

Update LoggingHandler to stringify record.msg #312

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

Merged
merged 2 commits into from
Nov 7, 2018

Conversation

axw
Copy link
Member

@axw axw commented Nov 1, 2018

Update LoggingHandler so that it converts the log record
message to a string. The message may be any arbitrary object,
and currently non-string messages are being sent as objects,
which the APM Server rejects.

Fixes #295

@jaap3
Copy link

jaap3 commented Nov 1, 2018

Are the build failures caused by this PR?

@axw axw changed the base branch from master to 3.x November 1, 2018 11:19
@beniwohli
Copy link
Contributor

Unfortunately, the builds on Travis and AppVeyor are somewhat broken at the moment. I hope to fix that soon. apm-ci is the most comprehensive build, so if that passes we should be OK.

A quick note on branches. Master has a bunch of backwards incompatible changes, and will be released as 4.0 together with APM Server 6.5. We will continue to cut maintenance releases of the 3.x line for some time. I can create a new 3.x with a backport of this PR some time next week (I'm traveling at the moment).

Sorry for dropping the ball on this issue, and thanks @axw for picking it up!

@axw
Copy link
Member Author

axw commented Nov 1, 2018

A quick note on branches. Master has a bunch of backwards incompatible changes, and will be released as 4.0 together with APM Server 6.5. We will continue to cut maintenance releases of the 3.x line for some time. I can create a new 3.x with a backport of this PR some time next week (I'm traveling at the moment).

I've targeted 3.x (targeting master was a mistake), so that @jaap3 can test.

There are failures in apm-ci, but they appear to have something to do with json-schema; nothing I touched.

@jaap3
Copy link

jaap3 commented Nov 1, 2018

@axw I've installed your branch, I'll keep it running for a while and report back if I see anything (earliest monday)

@jaap3
Copy link

jaap3 commented Nov 1, 2018

@beniwohli this is probably not the best place to discuss this, but, after the release of APM Server 6.5/Client 4.0 what would an upgrade path look like?

e.g. should all projects be upgraded to client version 4.0 immediately? Or can the server 6.5 handle messages from Client < 4.0? Or, inverted, can projects be safely upgraded to client 4.0 while the server is still < 6.5?

This is not a huge issue for me currently, but in the future I will probably have many project reporting to the same APM server. So I'm very interested in a smooth upgrade path where I don't have to worry about updating multiple things simultaneously.

From reading the CHANGELOG it seems that I should at least be pinning the client to <4 for now.

@beniwohli
Copy link
Contributor

@jaap3 regarding upgrading: APM Server 6.5 (and all coming 6.x versions) will have support for the v1 API, which the agent versions < 4.0 talk. So you should be able to upgrade your APM Server(s) first, and then upgrade the agents.

The other way around is unfortunately not possible, as it would have been way too complicated to support both v1 and the new v2 API in the agent.

Generally speaking, we highly suggest to pin the elastic-apm dependency to at least the major version. New major versions usually indicate a backwards incompatible change. We should probably add a note to the install docs about this...

axw and others added 2 commits November 7, 2018 10:24
Update LoggingHandler so that it converts the log record
message to a string. The message may be any arbitrary object,
and currently non-string messages are being sent as objects,
which the APM Server rejects.
@beniwohli beniwohli force-pushed the logging-msg-arbitrary-object branch from 785c21c to 5852623 Compare November 7, 2018 09:25
@beniwohli
Copy link
Contributor

@axw force-pushed a rebase on top of updated 3.x branch to your repo, hope that's ok 😊 will merge as soon as tests pass (travis ci will not pass due to unrelated breakage with django master)

@axw
Copy link
Member Author

axw commented Nov 7, 2018

@axw force-pushed a rebase on top of updated 3.x branch to your repo, hope that's ok blush will merge as soon as tests pass (travis ci will not pass due to unrelated breakage with django master)

Sure is. Thanks for taking this over.

@beniwohli beniwohli merged commit b767230 into elastic:3.x Nov 7, 2018
beniwohli pushed a commit that referenced this pull request Nov 7, 2018
* Update LoggingHandler to stringify record.msg

Update LoggingHandler so that it converts the log record
message to a string. The message may be any arbitrary object,
and currently non-string messages are being sent as objects,
which the APM Server rejects.

* added stringification to Logbook handler as well
@beniwohli
Copy link
Contributor

Merged into 3.x and cherry-picked into master. Thanks for the work on this @axw, and thanks @jaap3 for the report!

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.

3 participants