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.

Sign up for GitHub

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

Jump to bottom

Fix #340 bug #342

Merged
merged 2 commits into from
May 15, 2024
Merged

Fix #340 bug #342

merged 2 commits into from
May 15, 2024

Conversation

tkmsaaaam
Copy link
Contributor

@tkmsaaaam tkmsaaaam commented May 11, 2024 •
edited
Loading

Fix #340 's bug.

I made bellow changes.

1. Use appropriate CLOUD_PROVIDER .

2. Use appropriate CLOUD_PLATFORM.

3. Add test cases about GCP .

4. Fix to map resource when platform == null

I have to map to k8s resources when this condition. If i did this.

2. Use appropriate CLOUD_PLATFORM.

Then I need duplicate these lines https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/342/files#diff-992354fc4c9a5bcfac13812fb9e420d0498483ae56b986f25e71d6649386de99L187-L198 . So I make mapK8sResourceOrGenericTaskOrNode to refactor it.

@tkmsaaaam tkmsaaaam requested a review from a team as a code owner May 11, 2024 06:54
This was referenced May 11, 2024
Merged
Closed
@tkmsaaaam
Copy link
Contributor Author

I update description. #342 (comment)

@tkmsaaaam
Copy link
Contributor Author

Fix #340 's bug.

Use appropriate CLOUD_PROVIDER .
Add test cases about GCP .
Fix to map resource when platform == null
Regarding use of appropriate cloud provider, I think GCP is correct, since GCP has Monitored Resource types for other cloud providers like AWS too.

If it is true. I don't need changes about below. I will fix it.

1. Use appropriate CLOUD_PROVIDER .

@tkmsaaaam tkmsaaaam requested a review from psx95 May 12, 2024 05:28
@psx95 psx95 self-assigned this May 12, 2024
@psx95
Copy link
Contributor

psx95 commented May 12, 2024

Thank you for the clarification, I will review this shortly!

psx95
psx95 requested changes May 14, 2024
View reviewed changes
Copy link
Contributor

@psx95 psx95 left a comment

Choose a reason for hiding this comment

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

Thanks for reporting and fixing this bug!
Could you address the last pending comment

@tkmsaaaam
Copy link
Contributor Author

Thanks for reporting and fixing this bug!
Could you address the last pending comment

Thanks for your review. I've done it.

psx95
psx95 approved these changes May 15, 2024
View reviewed changes
Copy link
Contributor

@psx95 psx95 left a comment

Choose a reason for hiding this comment

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

/gcbrun

@psx95
Copy link
Contributor

psx95 commented May 15, 2024

/gcbrun

@psx95
Copy link
Contributor

psx95 commented May 15, 2024

Fix #340 's bug.
Use appropriate CLOUD_PROVIDER .
Add test cases about GCP .
Fix to map resource when platform == null
Regarding use of appropriate cloud provider, I think GCP is correct, since GCP has Monitored Resource types for other cloud providers like AWS too.

If it is true. I don't need changes about below. I will fix it.

1. Use appropriate CLOUD_PROVIDER .

For posterity, the CLOUD_PROVIDER resource attribute is not added to the MR resource, as such its value does not affect the mapped resource. However, the more appropriate CLOUD_PROVIDER values should be specific to the CLOUD_PLATFORM as indicated by the tests in this PR.

@psx95 psx95 merged commit 78c5cd4 into GoogleCloudPlatform:main May 15, 2024
18 checks passed
@tkmsaaaam tkmsaaaam deleted the fix-340 branch May 15, 2024 22:08
diegomarquezp referenced this pull request in googleapis/sdk-platform-java May 31, 2024
@renovate-bot
deps: update dependency com.google.cloud.opentelemetry:detector-resou…
6c1dbfc 
…rces-support to v0.29.0 (#2831)

[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[com.google.cloud.opentelemetry:detector-resources-support](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java)
| `0.28.0` -> `0.29.0` |
[![age](https://developer.mend.io/api/mc/badges/age/maven/com.google.cloud.opentelemetry:detector-resources-support/0.29.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/maven/com.google.cloud.opentelemetry:detector-resources-support/0.29.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/maven/com.google.cloud.opentelemetry:detector-resources-support/0.28.0/0.29.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/maven/com.google.cloud.opentelemetry:detector-resources-support/0.28.0/0.29.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>GoogleCloudPlatform/opentelemetry-operations-java
(com.google.cloud.opentelemetry:detector-resources-support)</summary>

###
[`v0.29.0`](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/releases/tag/v0.29.0)

[Compare
Source](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/compare/v0.28.0...v0.29.0)

#### Release Highlights

- Non-GKE resource now map to `k8s_*` monitored resource
([#&#8203;340](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/issues/340)
,
[#&#8203;342](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/issues/342))
- Support for mapping to custom Monitored Resources
([#&#8203;346](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/issues/346))
- Newly deprecated APIs:
[mapResource](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/blob/b0caafdaceb2b7418d256cecc05fa8fce9bafecc/exporters/metrics/src/main/java/com/google/cloud/opentelemetry/metric/ResourceTranslator.java#L36)

#### What's Changed

- Update maven-badges to link to sonatype central by
[@&#8203;psx95](https://togithub.com/psx95) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/322](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/322)
- Add auth support for gRPC by
[@&#8203;psx95](https://togithub.com/psx95) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/323](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/323)
- Switch to ConcurrentMap for thread-safety by
[@&#8203;psx95](https://togithub.com/psx95) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/328](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/328)
- Add otlp with spring boot sample by
[@&#8203;psx95](https://togithub.com/psx95) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/327](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/327)
- Change OTel quickstart to use log4j2 instead of logback by
[@&#8203;aabmass](https://togithub.com/aabmass) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/330](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/330)
- Always output logging.googleapis.com/trace_sampled as true by
[@&#8203;aabmass](https://togithub.com/aabmass) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/332](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/332)
- Update JVM flag otel.java.experimental.extension by
[@&#8203;psx95](https://togithub.com/psx95) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/338](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/338)
- Add test case for zone resource detection on GAE standard by
[@&#8203;aabmass](https://togithub.com/aabmass) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/341](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/341)
- Fix
[#&#8203;258](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/issues/258)
enable mapping when not gcp kubernetes by
[@&#8203;tkmsaaaam](https://togithub.com/tkmsaaaam) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/340](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/340)
- Fix
[#&#8203;340](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/issues/340)
bug by [@&#8203;tkmsaaaam](https://togithub.com/tkmsaaaam) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/342](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/342)
- Make all AttributeMappings final by
[@&#8203;psx95](https://togithub.com/psx95) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/347](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/347)
- Add support for custom MR mappings by
[@&#8203;psx95](https://togithub.com/psx95) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/346](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/346)

#### New Contributors

- [@&#8203;tkmsaaaam](https://togithub.com/tkmsaaaam) made their first
contribution in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/340](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/340)

**Full Changelog**:
GoogleCloudPlatform/opentelemetry-operations-java@v0.28.0...v0.29.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/googleapis/sdk-platform-java).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zNzcuOCIsInVwZGF0ZWRJblZlciI6IjM3LjM3Ny44IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@psx95 psx95 psx95 approved these changes

Assignees

@psx95 psx95

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tkmsaaaam @psx95