Skip to content

Conversation

@jalvz
Copy link
Contributor

@jalvz jalvz commented May 27, 2021

Implements part of https://github.com/elastic/apm-server/issues/5034

Checklist

How to test these changes

Send some events to apm-server, check that service.name is in the request logs

@jalvz jalvz added the v7.14.0 label May 27, 2021
@ghost
Copy link

ghost commented May 27, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #5355 updated

  • Start Time: 2021-05-27T13:14:15.745+0000

  • Duration: 38 min 34 sec

  • Commit: 55c757d

Test stats 🧪

Test Results
Failed 0
Passed 6262
Skipped 120
Total 6382

Trends 🧪

Image of Build Times

Image of Tests

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Explicitly adding service.name to every log message is going to be difficult and error-prone to maintain. Moreover, we should be adding the fields to log messages coming from libbeat too.

The service name is the beat name, and that's already known to logp, so can we add the field centrally in logp? It should be possible to do this with a zap.Fields option, or with zap.Logger.WithFields.

Given that we'll also need to set event.dataset as part of https://github.com/elastic/apm-server/issues/5034, maybe we should add an option in ecszap for setting both service.name and event.dataset?

@jalvz
Copy link
Contributor Author

jalvz commented May 28, 2021

The service name is the beat name

I understood it was the apm service name, not the beat name. The this doesn't make sense, closing.

@jalvz jalvz closed this May 28, 2021
@axw axw self-assigned this Jul 6, 2021
@axw
Copy link
Member

axw commented Jul 6, 2021

Tested changed made in elastic/beats#25998, using 7.14 BC1. Example apm-server log record:

localtesting_7.14.0_apm-server | {"log.level":"info","@timestamp":"2021-07-06T07:56:51.344Z","log.logger":"request","log.origin":{"file.name":"middleware/log_middleware.go","file.line":63},"message":"request ok","service.name":"apm-server","event.dataset":"apm-server","url.original":"/","http.request.method":"GET","user_agent.original":"curl/7.29.0","source.address":"127.0.0.1","http.request.body.bytes":0,"http.request.id":"bb9ec214-10f0-4c7a-95d9-1c48bcf50928","event.duration":97785,"http.response.status_code":200,"ecs.version":"1.6.0"}

Notably, service.name and event.dataset fields are there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants