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

Ensure ECS compliant logging when enabled. #3829

Merged
merged 19 commits into from Dec 15, 2020

Conversation

simitt
Copy link
Contributor

@simitt simitt commented May 28, 2020

Motivation/summary

Move towards ECS compliant logging within the APM Server code by changing the log format and switching logging.ecs on by default.

Log files created within the libbeat code might still not be ECS compliant.

** Do not merge this PR until we are sure we want to backport to 7.9 **

Checklist

- [ ] I have signed the Contributor License Agreement.

I have considered changes for:

How to test these changes

Start APM Server and observe log lines. Following changes should be applied by default:

  • logging.ecs is enabled, therefore ECS logs MVP fields are added
  • error -> error.message
  • stacktrace -> error.stacktrace
  • url -> url.original
  • request_id -> http.request.id
  • method -> http.request.method
  • content_length -> http.request.body.bytes
  • remote_address -> source.address
  • user-agent -> user_agent.original

Related issues

implements #3796

@apmmachine
Copy link
Collaborator

apmmachine commented May 28, 2020

💚 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 #3829 updated

  • Start Time: 2020-12-15T07:58:31.412+0000

  • Duration: 54 min 39 sec

Test stats 🧪

Test Results
Failed 0
Passed 4613
Skipped 124
Total 4737

Steps errors 3

Expand to view the steps failures

Compress
  • Took 0 min 0 sec . View more details on here
  • Description: tar --exclude=coverage-files.tgz -czf coverage-files.tgz coverage
Compress
  • Took 0 min 0 sec . View more details on here
  • Description: tar --exclude=system-tests-linux-files.tgz -czf system-tests-linux-files.tgz system-tests
Test Sync
  • Took 3 min 17 sec . View more details on here
  • Description: ./.ci/scripts/sync.sh

}, nil
}
}

func requestArgs(c *request.Context, ecsEnabled bool) ([]interface{}, error) {
Copy link

Choose a reason for hiding this comment

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

Instead of returning []interface and using Infow and Errorw consider to return a logger and add the labels via (*Logger).With.

@simitt simitt marked this pull request as ready for review June 16, 2020 09:38
@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2020

Codecov Report

Merging #3829 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3829   +/-   ##
=======================================
  Coverage   75.16%   75.16%           
=======================================
  Files         138      138           
  Lines        6629     6629           
=======================================
  Hits         4983     4983           
  Misses       1646     1646           

@cla-checker-service
Copy link

cla-checker-service bot commented Jun 17, 2020

💚 CLA has been signed

@simitt
Copy link
Contributor Author

simitt commented Jun 17, 2020

cla/check

@simitt simitt added the review label Jun 17, 2020
@simitt simitt requested a review from a team June 17, 2020 12:23
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.

Looks great overall, just one issue with error.stacktrace

beater/middleware/log_middleware.go Outdated Show resolved Hide resolved
beater/middleware/log_middleware.go Show resolved Hide resolved
beater/middleware/log_middleware.go Outdated Show resolved Hide resolved
beater/middleware/log_middleware.go Outdated Show resolved Hide resolved
beater/middleware/log_middleware.go Outdated Show resolved Hide resolved
beater/middleware/log_middleware.go Outdated Show resolved Hide resolved
@simitt
Copy link
Contributor Author

simitt commented Jul 2, 2020

jenkins run the hey-apm tests please

@simitt simitt added the 7.10.0 label Jul 6, 2020
@simitt simitt added 7.11.0 and removed 7.10.0 labels Oct 6, 2020
@simitt simitt added the v7.11.0 label Oct 29, 2020
@simitt simitt removed the 7.11.0 label Oct 29, 2020
@simitt simitt requested a review from axw December 4, 2020 17:08
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.

LGTM apart from the timestamp bit. Please merge away after that one is fixed.

systemtest/apmservertest/server.go Outdated Show resolved Hide resolved
systemtest/apmservertest/server.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #3829 (29b6d5e) into master (1fd28d8) will decrease coverage by 0.01%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##           master    #3829      +/-   ##
==========================================
- Coverage   75.95%   75.94%   -0.02%     
==========================================
  Files         161      161              
  Lines        9787     9789       +2     
==========================================
  Hits         7434     7434              
- Misses       2353     2355       +2     
Impacted Files Coverage Δ
beater/middleware/log_middleware.go 80.39% <78.57%> (-3.29%) ⬇️
...ack/apm-server/aggregation/txmetrics/aggregator.go 93.36% <0.00%> (ø)

@simitt simitt merged commit 3787214 into elastic:master Dec 15, 2020
simitt added a commit to simitt/apm-server that referenced this pull request Dec 15, 2020
If `logging.ecs` is set log data in ECS compliant way.

closes elastic#3796
simitt added a commit that referenced this pull request Dec 15, 2020
If `logging.ecs` is set log data in ECS compliant way.

closes #3796
@axw axw self-assigned this Dec 21, 2020
@axw
Copy link
Member

axw commented Dec 21, 2020

Tested with 7.11.0 BC1, using apm-integration-testing. By default, ECS is enabled.

{
  "@timestamp": "2020-12-21T06:06:39.084Z",
  "ecs.version": "1.6.0",
  "error.message": "404 page not found",
  "event.duration": 70288,
  "http.request.body.bytes": 0,
  "http.request.id": "419d573a82ecc524",
  "http.request.method": "GET",
  "http.response.status_code": 404,
  "log.level": "error",
  "log.logger": "request",
  "log.origin": {
    "file.line": 60,
    "file.name": "middleware/log_middleware.go"
  },
  "message": "404 page not found",
  "source.address": "172.29.0.1",
  "trace.id": "419d573a82ecc5245a96abbede83b173",
  "transaction.id": "419d573a82ecc524",
  "url.original": "/verloren",
  "user_agent.original": "curl/7.68.0"
}

http.request.id is not in ECS, but everything else is.

@graphaelli
Copy link
Member

Good reminder, we have been meaning to get this into ECS for a long time - elastic/ecs#1208

@axw
Copy link
Member

axw commented Dec 22, 2020

Good call on adding it, thanks!

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.

None yet

7 participants