Skip to content

Move agent config and its dependencies into agent classloader #322

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 3 commits into from
Jun 21, 2021

Conversation

pavolloffay
Copy link
Member

@pavolloffay pavolloffay commented Jun 17, 2021

Signed-off-by: Pavol Loffay [email protected]

Description

Resolves #313

Notable changes:

  • config and its dependencies are moved to agent classloarder. The classes in the agent classloader are totally isolated from the application.
  • shading for proto and jackson is removed - not needed anymore
  • added a new config interface that is backed by old config class. The new interface is located in javaagent-core that lives in the bootstrap classloader. The binding between the interface and proto-based config is done at javaagent bootstrap via component installer SPI.

The whole idea is that javaagent-core is located in the bootstrap classloader hence it should not bring any other 3rd party dependencies. The otel-extensions is located in the agent classloader and can load and use anything bc it is fully isolated from the application.

@pavolloffay pavolloffay changed the title Move agent config and its deps into agent classloader Move agent config and its dependencies into agent classloader Jun 17, 2021
@pavolloffay
Copy link
Member Author

@ryandens it seems that snyk still fails on the PRs from forks.

Signed-off-by: Pavol Loffay <[email protected]>
@pavolloffay
Copy link
Member Author

The CI passes not. Snyk is misconfigured and fails on PRs from forks on

"error-message": "snyk requires an authenticated account. Please run snyk auth and try again.",

@pavolloffay
Copy link
Member Author

The body capture works, tested on https://github.com/grpc/grpc-java/tree/master/examples

Screenshot of Jaeger UI

@pavolloffay pavolloffay requested review from ryandens and shashank11p and removed request for ryandens June 18, 2021 09:48
@pavolloffay
Copy link
Member Author

do not merge yet it needs more love - e.g. remove the filter paths from the instrumentation config. That should not be available to instrumentations.

@pavolloffay
Copy link
Member Author

@shashank11p PR updated, could you please re-review?

@ryandens
Copy link
Member

@ryandens it seems that snyk still fails on the PRs from forks.

Hey Pavol! Sorry about that would you mind trying out a fix to help this PR along? I think we need to

  1. Swap out --org=hypertrace with --org=$GITHUB_ACTOR
  2. Depending on whether you've done this before, you'll need to login and authorize your github account.

@pavolloffay
Copy link
Member Author

pavolloffay commented Jun 21, 2021

I am not sure if that will work, most likely the secret is not present in PRs from forks https://github.com/pavolloffay/javaagent/blob/52f0a2d388ce5951e4627f6f18c0fa0f9e4f12a1/.github/workflows/build.yaml#L82.

This repository has the same snyk setting as other HT repos e.g. https://github.com/hypertrace/hypertrace-service/blob/main/.github/workflows/pr-build.yml#L79. I will leave this as it is, somebody from the team should fix it so that PRs from the community are not turning red.

EDIT: snyk passed on hypertrace/hypertrace-service#97. I will look at it in a different PR

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.

GRPC Instrumentation not adding rpc.body to spans
3 participants