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

[processor/resourcedetectionprocessor] support openshift #16079

Conversation

frzifus
Copy link
Member

@frzifus frzifus commented Nov 4, 2022

Description:

This pr adds openshift support to the resourcedetectionprocessor.

Link to tracking Issue:
Closes #15694

Testing:
A few tests have been added.

The detector can be tested manually with the following configuration:

receivers:
  otlp:
    protocols:
      grpc:
      http:

processors:
  resourcedetection/openshift:
    detectors: openshift
    timeout: 2s
    override: false
    openshift:
      address: https://api.openshift.com
      token: <your-serviceaccount-token>

exporters:
  logging:
    logLevel: debug

service:
  pipelines:
    traces:
      processors: [resourcedetection/openshift]
      receivers: [otlp]
      exporters: [logging]

If the collector is rolled out in an OpenShift cluster, there is no need for user configuration. Note that the used serviceaccount must be authorized to read the api endpoints /apis/config.openshift.io/v1/infrastructures/{name}/status and /apis/config.openshift.io/v1/clusterversions/{name}/status.

Example:

{
  "cloud.platform": "aws_openshift",
  "cloud.provider": "aws",
  "cloud.region": "us-east-1",
  "k8s.cluster.name": "observability-test",
  "k8s.cluster.version": "v1.21.11+5cc9227",
  "openshift.cluster.version": "4.8.51"
}

Documentation:
Extended readme

@frzifus frzifus force-pushed the resourcedetectionprocessor_support_openshift branch from 2705c25 to c5bebfe Compare November 9, 2022 19:37
@frzifus
Copy link
Member Author

frzifus commented Nov 9, 2022

@pavolloffay @kevinearls would like to hear your thoughts on this.

@frzifus frzifus force-pushed the resourcedetectionprocessor_support_openshift branch 2 times, most recently from 0a4791c to db038b7 Compare November 9, 2022 23:59
@frzifus frzifus marked this pull request as ready for review November 10, 2022 00:38
@frzifus frzifus requested a review from a team November 10, 2022 00:38
@frzifus frzifus force-pushed the resourcedetectionprocessor_support_openshift branch 2 times, most recently from baa3499 to a98ebe2 Compare November 16, 2022 16:40
@github-actions github-actions bot requested a review from jrcamp November 16, 2022 16:40
@frzifus frzifus force-pushed the resourcedetectionprocessor_support_openshift branch from a98ebe2 to 8f263f1 Compare November 17, 2022 10:19
@frzifus
Copy link
Member Author

frzifus commented Nov 17, 2022

@Aneurysm9 @dashpole could you please have a look? (OT: seems there are some flaky tests on the mainline, could you retrigger it?)

}

// TODO(frzifus): make sure Platform matches the enum.
attrs.PutStr(conventions.AttributeCloudProvider, infra.Platform)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is what the comment is saying, but we need to update https://github.com/open-telemetry/opentelemetry-specification/blob/main/semantic_conventions/resource/cloud.yaml to include these new values.

Copy link
Member Author

Choose a reason for hiding this comment

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

right, i wasnt sure if it needs to part of the spec before we can continue here. I will work on that 👍

processor/resourcedetectionprocessor/README.md Outdated Show resolved Hide resolved
@github-actions github-actions bot requested a review from dashpole November 17, 2022 15:55
@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2022

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Dec 2, 2022
@frzifus frzifus force-pushed the resourcedetectionprocessor_support_openshift branch from 5f642f6 to 8afcd2d Compare December 7, 2022 15:22
@runforesight
Copy link

runforesight bot commented Dec 7, 2022

Foresight Summary

    
Major Impacts

build-and-test-windows duration(4 seconds) has decreased 44 minutes 6 seconds compared to main branch avg(44 minutes 10 seconds).
View More Details

⭕  build-and-test-windows workflow has finished in 4 seconds (44 minutes 6 seconds less than main branch avg.) and finished at 12th Jan, 2023.


Job Failed Steps Tests
windows-unittest-matrix -     🔗  N/A See Details
windows-unittest -     🔗  N/A See Details

✅  tracegen workflow has finished in 57 seconds (1 minute 36 seconds less than main branch avg.) and finished at 12th Jan, 2023.


Job Failed Steps Tests
build-dev -     🔗  N/A See Details
publish-latest -     🔗  N/A See Details
publish-stable -     🔗  N/A See Details

✅  changelog workflow has finished in 1 minute 19 seconds (56 seconds less than main branch avg.) and finished at 12th Jan, 2023.


Job Failed Steps Tests
changelog -     🔗  N/A See Details

✅  check-links workflow has finished in 1 minute 43 seconds and finished at 12th Jan, 2023.


Job Failed Steps Tests
changed files -     🔗  N/A See Details
check-links -     🔗  N/A See Details

✅  telemetrygen workflow has finished in 1 minute 6 seconds (1 minute 35 seconds less than main branch avg.) and finished at 12th Jan, 2023.


Job Failed Steps Tests
build-dev -     🔗  N/A See Details
publish-stable -     🔗  N/A See Details
publish-latest -     🔗  N/A See Details

✅  build-and-test workflow has finished in 41 minutes 31 seconds (7 minutes 36 seconds less than main branch avg.) and finished at 12th Jan, 2023.


Job Failed Steps Tests
unittest-matrix (1.19, internal) -     🔗  ✅ 650  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, extension) -     🔗  ✅ 528  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, internal) -     🔗  ✅ 650  ❌ 0  ⏭ 0    🔗 See Details
correctness-metrics -     🔗  ✅ 2  ❌ 0  ⏭ 0    🔗 See Details
correctness-traces -     🔗  ✅ 17  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, processor) -     🔗  ✅ 1488  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, processor) -     🔗  ✅ 1488  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, extension) -     🔗  ✅ 528  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, receiver-0) -     🔗  ✅ 2565  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, receiver-0) -     🔗  ✅ 2565  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, exporter) -     🔗  ✅ 2467  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, other) -     🔗  ✅ 4438  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, other) -     🔗  ✅ 4438  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, receiver-1) -     🔗  ✅ 1887  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, receiver-1) -     🔗  ✅ 1887  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, exporter) -     🔗  ✅ 2467  ❌ 0  ⏭ 0    🔗 See Details
integration-tests -     🔗  ✅ 55  ❌ 0  ⏭ 0    🔗 See Details
setup-environment -     🔗  N/A See Details
check-codeowners -     🔗  N/A See Details
check-collector-module-version -     🔗  N/A See Details
checks -     🔗  N/A See Details
lint-matrix (receiver-0) -     🔗  N/A See Details
lint-matrix (receiver-1) -     🔗  N/A See Details
lint-matrix (processor) -     🔗  N/A See Details
lint-matrix (exporter) -     🔗  N/A See Details
lint-matrix (extension) -     🔗  N/A See Details
lint-matrix (internal) -     🔗  N/A See Details
lint-matrix (other) -     🔗  N/A See Details
build-examples -     🔗  N/A See Details
unittest (1.19) -     🔗  N/A See Details
unittest (1.18) -     🔗  N/A See Details
lint -     🔗  N/A See Details
cross-compile (darwin, amd64) -     🔗  N/A See Details
cross-compile (darwin, arm64) -     🔗  N/A See Details
cross-compile (linux, amd64) -     🔗  N/A See Details
cross-compile (linux, 386) -     🔗  N/A See Details
cross-compile (linux, arm) -     🔗  N/A See Details
cross-compile (linux, arm64) -     🔗  N/A See Details
cross-compile (linux, ppc64le) -     🔗  N/A See Details
cross-compile (windows, 386) -     🔗  N/A See Details
cross-compile (windows, amd64) -     🔗  N/A See Details
build-package (deb) -     🔗  N/A See Details
build-package (rpm) -     🔗  N/A See Details
windows-msi -     🔗  N/A See Details
publish-check -     🔗  N/A See Details
publish-dev -     🔗  N/A See Details
publish-stable -     🔗  N/A See Details

✅  load-tests workflow has finished in 8 minutes 37 seconds (6 minutes 37 seconds less than main branch avg.) and finished at 12th Jan, 2023.


Job Failed Steps Tests
loadtest (TestTraceAttributesProcessor) -     🔗  ✅ 3  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestIdleMode) -     🔗  ✅ 1  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestMetric10kDPS|TestMetricsFromFile) -     🔗  ✅ 6  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestTraceNoBackend10kSPS|TestTrace1kSPSWithAttrs) -     🔗  ✅ 8  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestTraceBallast1kSPSWithAttrs|TestTraceBallast1kSPSAddAttrs) -     🔗  ✅ 10  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestMetricResourceProcessor|TestTrace10kSPS) -     🔗  ✅ 12  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestBallastMemory|TestLog10kDPS) -     🔗  ✅ 19  ❌ 0  ⏭ 0    🔗 See Details
setup-environment -     🔗  N/A See Details

✅  prometheus-compliance-tests workflow has finished in 13 minutes 24 seconds (⚠️ 5 minutes 15 seconds more than main branch avg.) and finished at 12th Jan, 2023.


Job Failed Steps Tests
prometheus-compliance-tests -     🔗  ✅ 21  ❌ 0  ⏭ 0    🔗 See Details

🔎 See details on Foresight

*You can configure Foresight comments in your organization settings page.

@frzifus frzifus force-pushed the resourcedetectionprocessor_support_openshift branch from 8afcd2d to bda14a1 Compare December 7, 2022 15:35
@github-actions github-actions bot removed the Stale label Dec 8, 2022
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@frzifus frzifus force-pushed the resourcedetectionprocessor_support_openshift branch from d471a60 to 294fb7e Compare January 11, 2023 14:08
@frzifus
Copy link
Member Author

frzifus commented Jan 11, 2023

@dashpole, could you please take another look? :)

Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

Code changes lgtm.

Before we merge this, we should identify CODEOWNERS for the openshift detection logic. Are you interested in being a CODEOWNER for the openshift resource detection portion?

@frzifus frzifus force-pushed the resourcedetectionprocessor_support_openshift branch 2 times, most recently from 41894a1 to 8639352 Compare January 12, 2023 09:00
@frzifus
Copy link
Member Author

frzifus commented Jan 12, 2023

Thanks for the quick review :)

Before we merge this, we should identify CODEOWNERS for the openshift detection logic. Are you interested in being a CODEOWNER for the openshift resource detection portion?

sure, have added me to the codeowners file. Thats right?

@frzifus frzifus requested a review from dashpole January 12, 2023 18:47
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

It looks like some of the checks are still failing

@frzifus frzifus force-pushed the resourcedetectionprocessor_support_openshift branch from 8639352 to 970a334 Compare January 12, 2023 22:30
@github-actions github-actions bot requested a review from dashpole January 12, 2023 22:31
Signed-off-by: Benedikt Bongartz <[email protected]>
@frzifus frzifus requested a review from codeboten January 12, 2023 22:45
@frzifus
Copy link
Member Author

frzifus commented Jan 12, 2023

It looks like some of the checks are still failing

Seems that the issue templates were not up to date and a removed variable was not committed. 😮‍💨

@frzifus
Copy link
Member Author

frzifus commented Jan 16, 2023

@codeboten is there something else blocking this?

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

LGTM, @dashpole not sure what happened to your approval, can you please have one more look.

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.

[processor/resourcedetectionprocessor] support openshift as metadata provider
6 participants