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

package cloud resource in javaagent - but keep them disabled by default #10754

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented Mar 5, 2024

plugins can be enabled or disabled using otel.resource.providers.<>.enabled, e.g. otel.resource.providers.aws.enabled

Fixes #10447

@zeitlinger zeitlinger self-assigned this Mar 5, 2024
@zeitlinger zeitlinger requested a review from a team March 5, 2024 12:12
@github-actions github-actions bot requested a review from theletterf March 5, 2024 12:36
Copy link
Contributor

@laurit laurit left a comment

Choose a reason for hiding this comment

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

I also created a PR to add the aws detectors a while back #7927 That one has a test that verifies that aws provider runs after ContainerResourceProvider could you check whether it would make sense to bring that test over to this PR.

@zeitlinger
Copy link
Member Author

I also created a PR to add the aws detectors a while back #7927 That one has a test that verifies that aws provider runs after ContainerResourceProvider could you check whether it would make sense to bring that test over to this PR.

@laurit done - and all the other suggestions incorporated

// Renovate's default behavior is only to update from unstable -> unstable if it's for the
// major.minor.patch, under the assumption that you would want to update to the stable version
// of that release instead of the unstable version for a future release
// (TODO remove once the artifacts above release stable versions)
"ignoreUnstable": false
"matchCurrentVersion": "/^.*-alpha$/",
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

extracted to standalone PR: #10761

Copy link
Contributor

Choose a reason for hiding this comment

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

as this part is already in a separate PR it might be best to remove it from this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@zeitlinger zeitlinger force-pushed the add-cloud-resource-providers-disabled-by-default branch from 4da18f8 to bca96f1 Compare March 5, 2024 14:43
@laurit laurit added this to the v2.2.0 milestone Mar 6, 2024
@zeitlinger zeitlinger force-pushed the add-cloud-resource-providers-disabled-by-default branch from 602ee35 to f412cae Compare March 11, 2024 11:32
@github-actions github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Mar 11, 2024
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

can you add documentation somewhere (either in this repo, or draft PR to website repo)?

@zeitlinger zeitlinger force-pushed the add-cloud-resource-providers-disabled-by-default branch from b68105d to 0bb07e8 Compare March 12, 2024 07:56
@zeitlinger
Copy link
Member Author

can you add documentation somewhere (either in this repo, or draft PR to website repo)?

open-telemetry/opentelemetry.io#4138

@zeitlinger zeitlinger force-pushed the add-cloud-resource-providers-disabled-by-default branch from 0bb07e8 to 694ab74 Compare March 12, 2024 10:21
@zeitlinger zeitlinger mentioned this pull request Mar 12, 2024
2 tasks
@laurit laurit merged commit 60d53e1 into open-telemetry:main Mar 12, 2024
49 checks passed
@zeitlinger zeitlinger deleted the add-cloud-resource-providers-disabled-by-default branch March 12, 2024 12:19
zzhlogin added a commit to aws-observability/aws-otel-java-instrumentation that referenced this pull request Dec 19, 2024
*Description of changes:*
Onboarding ADOT Java to v2.x, with the latest
`opentelemetry-java-instrumentation` dependency version `v2.10.0`.
Update the workflows, adot core implementation codes, contract tests and
smoke tests with following change:
**A.  Workflows:**
1. Update `.github/patches/opentelemetry-java-instrumentation.patch`
file with the same code change as previous patch based on upstream
v2.10.0.
2. Upgrade contract testing build env with JDK 23.

**B. Core repo:**
1. Upgrade `com.github.johnrengelman.shadow` to `com.gradleup.shadow` as
it has been marked under maintenance mode:
https://github.com/GradleUp/shadow?tab=readme-ov-file#gradle-shadow
2. Upgrade to new ktlint version 1.4.0, and apply latest spotless
format.
3. Upgrade gradle to 8.10
4. Upgrade `com.diffplug.spotless` to newer version 6.25.0
5. Upgrade `com.google.cloud.tools.jib` to newer version 3.4.4
6. Update the unit test to check the migrated semantic conventions.
7. AWS Resource detector is [set as disabled by
default](open-telemetry/opentelemetry-java-instrumentation#10754),
enable it by setting environment variable
`"otel.resource.providers.aws.enabled"` to be `true`.

**C. Contract tests:**
1. Use the latest `kotlin("jvm")` version to be compatible if the new
Java class from upstream.
2. Add `OTEL_EXPORTER_OTLP_PROTOCOL` to `grpc` for instrumentation as in
[version
v2.0.0](https://github.com/open-telemetry/opentelemetry-java-instrumentation/releases/tag/v2.0.0):
> The default OTLP protocol has been changed from grpc to http/protobuf
in orderto align with the specification.
3. Update tomcat contract test `TOMCAT_THREADS` metric threshold to
`-2`, as the value can be negative by [tomcat
design](https://github.com/apache/tomcat/blob/1afe41491f0e56ec0a776db5ff84607f87ce6640/java/org/apache/tomcat/util/net/AbstractEndpoint.java#L1204).
4. Update the test classes to migrate to the new semantic conventions to
match with [upstream latest
change](open-telemetry/opentelemetry-java-instrumentation@7cd705b):
a. All the change made in
#972:
      `http.url` -> `url.full`
      `http.method` -> `http.request.method`
      `http.status_code` -> `http.response.status_code`
      `net.peer.name` -> `server.address`
      `net.peer.port` -> `server.port`
   b. extra semantic conventions included in contract tests:
      `http.scheme` -> `url.scheme`
      `http.target` -> `url.query`
      `http.url` -> `url.full`
      `net.host.name` -> `server.address`
      `net.host.port` -> `server.port`
      `net.peer.name` -> `server.address`
      `net.peer.port` -> `server.port`
      `net.protocol.name` -> `network.protocol.name`
      `net.protocol.version` -> `network.protocol.version`
      `net.sock.host.addr` -> `network.local.address`
      `net.sock.host.port` -> `network.local.port`
      `net.sock.peer.addr` -> `network.peer.address`
      `net.sock.peer.name` -> no replacement, removed
`messaging.kafka.destination.partition` ->
`messaging.destination.partition.id`
`messaging.message.payload_size_bytes` -> `messaging.message.body.size`
Remove `network.protocol.name` attribute check as it has marked as
Conditionally required if not http and network.protocol.version is set:
https://opentelemetry.io/blog/2023/http-conventions-declared-stable/#summary-of-changes.
Conditionally check `peer.service` for http client. Match with PR:
https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/12083/files
Remove local socket attributes from http server span check as it is not
extracted from HttpServerAttributesExtractor
[code](https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/5ebb81b8a8ac0e5b3c9f2e175b847a3d0b12251f/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/semconv/http/HttpServerAttributesExtractorBuilder.java#L141),
Remove `http.response.header.content_length` as it need an explicit
configuration:
https://opentelemetry.io/docs/specs/semconv/attributes-registry/http/#http-response-header

**D. Smoke tests**
Enable controller telemetry using env variable
`OTEL_INSTRUMENTATION_COMMON_EXPERIMENTAL_CONTROLLER_TELEMETRY_ENABLED`,
which is disabled by default.

https://opentelemetry.io/docs/zero-code/java/agent/disable/#suppressing-controller-andor-view-spans


**Testing:**
1. All contract tests and smoke tests passed.
2. All workflow passed.
3. Tested all E2E tests (This E2E test PR need to be merged after ADOT
Java v2.x released):
- EC2:
https://github.com/aws-observability/aws-application-signals-test-framework/actions/runs/12378967612
- EKS:
https://github.com/aws-observability/aws-application-signals-test-framework/actions/runs/12382416662
- K8S:
https://github.com/aws-observability/aws-application-signals-test-framework/actions/runs/12378972156
- ECS:
https://github.com/aws-observability/aws-application-signals-test-framework/actions/runs/12378968843

4. Audit all upstream v2.0.0 - v2.10.0 [change
logs](https://github.com/open-telemetry/opentelemetry-java-instrumentation/releases)
and manual did E2E test and compared the traces, logs, metrics for Java
v1.x vs Java v2.x:
 
![Screenshot 2024-12-16 at 3 27
02 PM](https://github.com/user-attachments/assets/52bb9106-b4a1-4223-9df5-74f1f1b10089)


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test native This label can be applied to PRs to trigger them to run native tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add cloud provider resource detectors to Spring Starter
3 participants