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

Attacher improvements #1667

Merged
merged 33 commits into from
Mar 24, 2021
Merged

Conversation

felixbarny
Copy link
Member

@felixbarny felixbarny commented Feb 15, 2021

What does this PR do?

From changelog.asciidoc:

  • Overhaul of the <<setup-attach-cli,attacher cli>> application that allows to attach the agent to running JVMS
    • The artifact of the standalone cli applicaiton is now called apm-agent-attach-cli. The attacher API is still called apm-agent-attach.
    • There is also a slim version of the cli application that does not bundle the Java agent.
      It requires the --agent-jar option to be set.
    • Improved logging
      The application uses {ecs-logging-java-ref}/intro.html[Java ECS logging] to emit JSON logs.
      The log level can be configured with the --log-level option.
      By default, the program is logging to the console but using the --log-file option, it can also log to a file.
    • Attach to JVMs running under a different user (unix only)
      The JVM requires the attacher to be running under the same user as the target VM (the attachee).
      The apm-agent-attach-standalone.jar can now be run with a user that has permissions to switch to the user that runs the target VM.
      On Windows, the attacher can still only attach to JVMs that are running with under the same user.
    • New include/exclude discovery rules
      • --include-all: Attach to all discovered JVMs. If no matchers are provided, it will not attach to any JVMs.
      • --include-user/--exclude-user: Attach to all JVMs of a given operating system user.
      • --include-main/--exclude-main: Attach to all JVMs that whose main class/jar name, or system properties match the provided regex.
      • --include-vmargs/--exclude-vmargs: Attach to all JVMs that whose main class/jar name, or system properties match the provided regex.
    • Removal of options
      • The deprecated --arg option has been removed.
      • The -i/--include, -e/exclude options have been removed in favor of the --<include|exclude>-<main|vmargs> options.
      • The -p/--pid options have been removed in favor of the --include-pid option.
    • Changed behavior of the -l/--list option
      The option now only lists JVMs that match the include/exclude discovery rules.
      Thus, it can be used to do a dry-run of the matchers without actually performing an attachment.
      It even works in combination with --continuous now.
      By default, the VM arguments are not printed, but only when the -a/--list-vmargs option is set.
    • Remove dependency on jps
      Even when matching on the main class name or on system properties,
    • Checks the Java version before attaching to avoid attachment on unsupported JVMs.

Checklist

  • 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
  • This is a bugfix
  • This is a new plugin
    • I have updated CHANGELOG.asciidoc
    • My code follows the style guidelines of this project
    • I have made corresponding changes to the documentation
    • I have added tests that prove my fix is effective or that my feature works
    • New and existing unit tests pass locally with my changes
    • I have updated supported-technologies.asciidoc
    • Added an API method or config option? Document in which version this will be introduced
    • Added an instrumentation plugin? Describe how you made sure that old, non-supported versions are not instrumented by accident.
  • This is something else

TODOs

  • The apm-agent-attach.jar (attacher API) now has a compile dependency to log4j2 and log4j2-ecs-layout. Maybe we'll have to create different modules for the standalone cli attacher and the attacher API.
  • allow to log to a file
  • Allow specifying the path to the agent jar. Consider publishing an attacher that doesn't bundle the agent jar.
  • Rethink include/exclude rule logic. Currently, all conditions are applied with an OR. Example: --include-cmd foo --exclude-cmd bar would match foo-bar.jar. Should a given JVM pass the filter if it matches any include rule AND not matches any exclude rule?
    For Elastic Agent, they have recently added a similar rule logic. The following only allows APM to run:
constraints:
- rule: allow
  input: apm
- rule: deny
  input: "*"

Related issues

Closes #1688

- Attaching to a VM running under a different user works now
- Refactor discovery rules
- Add user include/exclude rules
- The --list option only lists the main class name, without the JVM arguments
- The -v option includes the JVM arguments
- Remove deprecated --args option
- Reduce risk of exposing sensitive data in heap dumps
- Use ps-based java process discovery as the last resort as it can kill non-Java processes
Revert to compiling premain module with Java 7 instead of Java 6,
as we're checking the Java version before attaching now.
Only the -cli module has a dependency on log4j and ecs logging
The apm-agent-attach (aka attach API) doesn't contain the CLI app now.
@codecov-io
Copy link

codecov-io commented Feb 15, 2021

Codecov Report

Merging #1667 (dfff7d9) into master (ffbaaa0) will increase coverage by 3.95%.
The diff coverage is 39.05%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1667      +/-   ##
============================================
+ Coverage     58.77%   62.73%   +3.95%     
- Complexity       92     3531    +3439     
============================================
  Files           403      364      -39     
  Lines         18475    17457    -1018     
  Branches       2570     2395     -175     
============================================
+ Hits          10858    10951      +93     
+ Misses         6847     5731    -1116     
- Partials        770      775       +5     
Impacted Files Coverage Δ Complexity Δ
.../main/java/co/elastic/apm/attach/UserRegistry.java 20.51% <20.51%> (ø) 6.00 <6.00> (?)
...main/java/co/elastic/apm/attach/AgentAttacher.java 28.28% <28.28%> (ø) 0.00 <0.00> (?)
...java/co/elastic/apm/attach/GetAgentProperties.java 36.66% <36.66%> (ø) 2.00 <2.00> (?)
...main/java/co/elastic/apm/attach/JvmDiscoverer.java 39.74% <39.74%> (ø) 0.00 <0.00> (?)
...i/src/main/java/co/elastic/apm/attach/JvmInfo.java 72.22% <72.22%> (ø) 12.00 <12.00> (?)
...ain/java/co/elastic/apm/attach/DiscoveryRules.java 80.23% <80.23%> (ø) 23.00 <23.00> (?)
...t/kafka/helper/KafkaInstrumentationHelperImpl.java
.../helper/KafkaInstrumentationHeadersHelperImpl.java
.../opentracing/impl/ScopeManagerInstrumentation.java
...agent/bci/IndyBootstrapDispatcherModuleSetter.java
... and 47 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ffbaaa0...dfff7d9. Read the comment docs.

@apmmachine
Copy link
Contributor

apmmachine commented Feb 15, 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 #1667 updated

  • Start Time: 2021-03-24T09:19:58.038+0000

  • Duration: 52 min 1 sec

  • Commit: 687a341

Test stats 🧪

Test Results
Failed 0
Passed 1912
Skipped 19
Total 1931

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 1912
Skipped 19
Total 1931

@SylvainJuge
Copy link
Member

SylvainJuge commented Feb 17, 2021

--pid is deprecated in favor of --include-pid

Seems to contradict

The -p/--pid options have been removed in favor of the --include-pid option.

Given the number of changes here, breaking compatibility with the --pid option could be a relevant option to ensure that other changes are not silently ignored on upgrade.

Also, most of the support cases we had in the past with --include/--exclude were due to the fact that it was relying on jps output parsing which isn't the most straightforward.

What exactly would be the use-case of the attacher without bundled agent in practice ? Having them bundled together tends to simplify deployment and compatibility checks.

@felixbarny
Copy link
Member Author

--pid is deprecated in favor of --include-pid

That's outdated. I removed -p/--pid.

What exactly would be the use-case of the attacher without bundled agent in practice ? Having them bundled together tends to simplify deployment and compatibility checks.

I think that this is the mode we'll be using when integrating APM Server. While we want to be able to update the Java agent independently from APM Server, there is a strong dependency between the UI, the server and the attacher-cli. These three components have to have a known set of features. If we were to implement a new type of include/exclude matcher in the attacher-cli in the future, we couldn't easily expose it in the UI if different versions of the attacher-cli may be used.

So, we'd bundle the attacher-cli-slim into APM Server and download the agent jar on demand, based on what's configured in the Fleet UI.

@felixbarny
Copy link
Member Author

Note that there's still the non-slim version of the attacher-cli that does ship with a bundled agent jar.

@SylvainJuge SylvainJuge added the enhancement Enhancement of an existing feature label Mar 1, 2021
@SylvainJuge SylvainJuge self-assigned this Mar 15, 2021
Copy link
Member

@SylvainJuge SylvainJuge left a comment

Choose a reason for hiding this comment

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

LGTM, only few comments/questions.
Will definitely make attachment easier, thus a breaking change is welcome for once !

@felixbarny felixbarny merged commit 5c73fc0 into elastic:master Mar 24, 2021
@felixbarny felixbarny deleted the attacher-improvements branch March 24, 2021 12:43
v1v added a commit to v1v/apm-agent-java that referenced this pull request Apr 7, 2021
…va into feature/java-stable-tag

* 'feature/java-stable-tag' of github.com:v1v/apm-agent-java: (534 commits)
  Added Support for JBoss Logging to the log correlation plugin (elastic#1737)
  Removing improper RabbitMQ reference from instrumentation class (elastic#1745)
  Loading Advice classes lazily when required (elastic#1736)
  add support for ibm JVMs (elastic#1739)
  Adds circuit breaker to app deployment in load pipeline (elastic#1731)
  Bump json-schema-validator from 1.0.50 to 1.0.51 (elastic#1732)
  Bump java-driver-core from 4.10.0 to 4.11.0 (elastic#1733)
  Cassandra instrumentation (elastic#1712)
  Build apm-agent-attach-cli before application-server-integration-tests (elastic#1729)
  Fix thread pool method matchers (elastic#1717)
  Always use English locale when formatting doubles (elastic#1727) (elastic#1728)
  feat: grab locust metrics on load tests (elastic#1724)
  log readonly msg errors only in debug (elastic#1715)
  feat: use metricbeat to grab host metrics (elastic#1718)
  Eliminate log4j shaders dependency in slf4j (elastic#1723)
  Fix log correlation for log4j2 (elastic#1720)
  Allowing more time for the test
  Bump json-schema-validator from 1.0.49 to 1.0.50 (elastic#1711)
  Fix master build and print URL on connection error
  Attacher improvements (elastic#1667)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-java enhancement Enhancement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RemoteAttacher: control log level
4 participants