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

set service name & version in ecs-logging #3064

Merged
merged 29 commits into from
Mar 28, 2023

Conversation

SylvainJuge
Copy link
Member

@SylvainJuge SylvainJuge commented Mar 21, 2023

Fixes #1626

What does this PR do?

When ECS logging library is used, the agent should provide fallback values if not explicitly set for the following fields

  • service.name
  • service.version

For now service.environment is not covered and will be handled as a follow-up PR with #3051 .

This behavior is described in more detail in agent specification for log correlation.

The code is a bit verbose and repetitive due to covering the following

  • testing multiple ecs-logging versions, 1.3.2, 1.4.0 and 1.5.0 through integration tests
  • setting service.version is only supported in 1.4.0 and later, thus it requires dedicated instrumentation and tests

I decided to simplify the instrumentation groups to a single logging-ecs (in addition to the existing logging group), as there is usually at most one logging library involved in a given application and there is very little risk to break anything here (so disabling is even less likely to be used). This could be added to the changelog as a "potentially breaking change" in the case where some existing users relied on the value that was used previously (log4j2-ecs).
Update: added as a discussion item to agree on.

Instrumentation group names (to be discussed during review)

This could definitely be handled in a separate PR/issue if needed.

For log-related instrumentations we have the following instrumentation groups:

  • logging added to all log-related instrumentations
  • <framework>-correlation to enable log-correlation, examples include jul-correlation, log4j1-correlation, log4j2-correlation, ...
  • <framework>-ecs for ecs-reformatting, examples include log4j1-ecs, log4j2-ecs, ...
  • <framework>-ecs to set service correlation fields (name, version and soon environment)
  • <framework>-error for error capture, examples include slf4j-error

This currently produces the current list of instrumentation groups:

  • jboss-logging-correlation
  • jul-ecs
  • jul-error
  • log4j1-correlation
  • log4j1-ecs
  • log4j1-error
  • log4j2-correlation
  • log4j2-ecs
  • log4j2-error
  • logback-correlation
  • logback-ecs
  • logging
  • logging-ecs
  • slf4j-error
  • tomcat-ecs

This list is quite long and naming is a bit confusing to link to features, also there is very rarely multiple distinct log frameworks used in the same application, so the need to fine-tune instrumentation of log4j1 or log4j2 should definitely be quite rare.

I would suggest to simplify by using the following instrumentation groups by removing the framework and focus on features:

  • logging for all log-related instrumentations (as it currently is)
  • log-correlation : log-correlation
  • log-ecs: log-reformatting to ECS (or rename it to log-reformatting to remove he ECS implementation detail).
  • log-error : error capture

Also, making the agent provide a value for service name and version (and soon environment) should be part of log-correlation as it allows to correlate logs.

Checklist

  • agree on a consistent naming strategy for logging-related instrumentation groups (as list expands linear to features x logging frameworks).
  • This is an enhancement of existing features, or a new feature in existing plugins
    • I have updated CHANGELOG.asciidoc
    • I have added tests that prove my fix is effective or that my feature works
    • Added an API method or config option? Document in which version this will be introduced
    • I have made corresponding changes to the documentation

@SylvainJuge SylvainJuge self-assigned this Mar 21, 2023
@apmmachine
Copy link
Contributor

apmmachine commented Mar 21, 2023

💚 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 previewSnapshots

Expand to view the summary

Build stats

  • Start Time: 2023-03-28T13:04:18.404+0000

  • Duration: 18 min 41 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark tests.

  • run jdk compatibility tests : Run the JDK Compatibility tests.

  • run integration tests : Run the Agent Integration tests.

  • run end-to-end tests : Run the APM-ITs.

  • run windows tests : Build & tests on windows.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@SylvainJuge SylvainJuge marked this pull request as ready for review March 22, 2023 14:49
@github-actions
Copy link

/test

@SylvainJuge SylvainJuge requested a review from JonasKunz March 23, 2023 16:09
Copy link
Contributor

@JonasKunz JonasKunz left a comment

Choose a reason for hiding this comment

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

Clean and nicely understandable solution!

@JonasKunz
Copy link
Contributor

I would suggest to simplify by using the following instrumentation groups by removing the framework and focus on features:

logging for all log-related instrumentations (as it currently is)
log-correlation : log-correlation
log-ecs: log-reformatting to ECS (or rename it to log-reformatting to remove he ECS implementation detail).
log-error : error capture
Also, making the agent provide a value for service name and version (and soon environment) should be part of log-correlation as it allows to correlate logs.

Sounds very reasonable to me. As you said, it is very unlikely that two logging frameworks are used within the same JVM (except JUL + other maybe) and it's even more unlikely that users would want to disable a feature for one framework but no the other.

I would suggest you go with the groups you proposed and mention it as "Potentially breaking change" in the changelog.

@SylvainJuge SylvainJuge merged commit 99aed16 into elastic:main Mar 28, 2023
@SylvainJuge SylvainJuge deleted the log-service-name branch March 28, 2023 13:51
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.

Instrument ecs-logging-java to set service name
3 participants