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

OpAMP Agent Extension #16594

Merged
merged 70 commits into from
Oct 26, 2023
Merged

Conversation

@portertech
Copy link
Contributor Author

portertech commented Dec 2, 2022

@tigrannajaryan @dmitryax am I on the right track here? Still very early/raw 😅 Based on the opamp-go examples and my configmap provider POC.

@runforesight
Copy link

runforesight bot commented Dec 3, 2022

Foresight Summary

    
Major Impacts

build-and-test-windows duration(3 seconds) has decreased 40 minutes 8 seconds compared to main branch avg(40 minutes 11 seconds).
View More Details

✅  check-links workflow has finished in 51 seconds (1 minute 9 seconds less than main branch avg.) and finished at 13th Jan, 2023.


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

✅  tracegen workflow has finished in 1 minute 18 seconds (1 minute 15 seconds less than main branch avg.) and finished at 13th Jan, 2023.


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

✅  prometheus-compliance-tests workflow has finished in 10 minutes 33 seconds (⚠️ 2 minutes 35 seconds more than main branch avg.) and finished at 13th Jan, 2023.


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

❌  build-and-test workflow has finished in 24 minutes 33 seconds (24 minutes 29 seconds less than main branch avg.) and finished at 13th Jan, 2023. 5 jobs failed.


Job Failed Steps Tests
correctness-metrics -     🔗  ✅ 2  ❌ 0  ⏭ 0    🔗 See Details
correctness-traces -     🔗  ✅ 17  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, internal) -     🔗  ✅ 597  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, processor) -     🔗  ✅ 1476  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, extension) -     🔗  ✅ 539  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, internal) -     🔗  ✅ 597  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, extension) -     🔗  ✅ 539  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, processor) -     🔗  ✅ 1476  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, receiver-0) -     🔗  ✅ 2563  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, receiver-0) -     🔗  ✅ 2563  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, exporter) -     🔗  ✅ 2450  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, exporter) -     🔗  ✅ 2450  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, other) -     🔗  ✅ 4395  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.18, receiver-1) -     🔗  ✅ 1886  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, other) -     🔗  ✅ 4395  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, receiver-1) -     🔗  ✅ 1886  ❌ 0  ⏭ 0    🔗 See Details
integration-tests -     🔗  ✅ 53  ❌ 0  ⏭ 0    🔗 See Details
setup-environment -     🔗  N/A See Details
build-examples -     🔗  N/A See Details
check-codeowners Check Code Owner Existence     🔗  N/A See Details
check-collector-module-version Check Collector Module Version     🔗  N/A See Details
checks Porto     🔗  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) Lint     🔗  N/A See Details
lint-matrix (other) -     🔗  N/A See Details
lint-matrix (internal) -     🔗  N/A See Details
lint Interpret result     🔗  N/A See Details
unittest (1.19) -     🔗  N/A See Details
unittest (1.18) -     🔗  N/A See Details
cross-compile -     🔗  N/A See Details
build-package -     🔗  N/A See Details
windows-msi -     🔗  N/A See Details
publish-stable -     🔗  N/A See Details
publish-check -     🔗  N/A See Details
publish-dev -     🔗  N/A See Details

✅  load-tests workflow has finished in 19 minutes 43 seconds (⚠️ 4 minutes 41 seconds more than main branch avg.) and finished at 13th Jan, 2023.


Job Failed Steps Tests
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
loadtest (TestIdleMode) -     🔗  N/A See Details
loadtest (TestMetric10kDPS|TestMetricsFromFile) -     🔗  N/A See Details
loadtest (TestTraceNoBackend10kSPS|TestTrace1kSPSWithAttrs) -     🔗  N/A See Details
loadtest (TestTraceAttributesProcessor) -     🔗  N/A See Details

⭕  build-and-test-windows workflow has finished in 3 seconds (40 minutes 8 seconds less than main branch avg.) and finished at 6th Feb, 2023.


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

✅  changelog workflow has finished in 2 minutes 21 seconds and finished at 6th Feb, 2023.


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

🔎 See details on Foresight

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

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

This is a good start, thanks!

.chloggen/add-opamp-agent-extension.yaml Show resolved Hide resolved
config.ExtensionSettings `mapstructure:",squash"` // squash ensures fields are correctly decoded in embedded struct

// Endpoint is the OpAMP server URL. Transport based on the scheme of the URL.
Endpoint string `mapstructure:"endpoint"`
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to explore if we can use HTTPClientSettings here. That will give us the ability to specify a bunch of client-side settings (headers, TLS, auth, etc). I don't know though if it works with WebSocket.
It probably is not going to help much since we can't use HTTPClientSettings.ToClient() anyway (and that is the most useful part of HTTPClientSettings), unless we somehow modify the OpAMP Client implementation to accept a pre-built HTTP Client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest using the HTTPClientSettings that way auth is provided for free.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using HTTPClientSettings feels cumbersome and perhaps its going to be problematic. StartSettings (https://github.com/open-telemetry/opamp-go/blob/main/client/types/startsettings.go#L11) has me doing things like: 924ed2d. Also, most of the settings will go unused, which could be confusing since they will pass validation.

extension/opampextension/config.go Outdated Show resolved Hide resolved
extension/opampextension/opamp_agent.go Outdated Show resolved Hide resolved
extension/opampextension/opamp_agent.go Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

Closes #16462

I have split #16462 into smaller tasks so that it is easier to deliver the PRs incrementally.

@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added Stale and removed Stale labels Jan 13, 2023
@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

github-actions bot commented Jun 3, 2023

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 Jun 3, 2023
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Jun 17, 2023
@mwear
Copy link
Member

mwear commented Jun 27, 2023

@portertech I'm curious if this is something you'd be interested in dusting off and working on in the near future? If not, this might be something I can look into trying to pick up.

@portertech
Copy link
Contributor Author

@mwear I was in a holding pattern for a few other changes. I do want to dust it off in the near future.

@tigrannajaryan
Copy link
Member

@mwear there is also a bunch of other related open issues if you are interested: #16462

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, just a few minor comments.

extension/opampextension/opamp_agent.go Show resolved Hide resolved
extension/opampextension/opamp_agent.go Show resolved Hide resolved
extension/opampextension/opamp_agent.go Outdated Show resolved Hide resolved
extension/opampextension/opamp_agent.go Outdated Show resolved Hide resolved
- `ws`: The OpAMP websocket transport settings.
- `tls`: TLS settings.
- `headers`: HTTP headers to set.
- `instance_uid`: A ULID formatted as a 26 character string in canonical
Copy link
Contributor

Choose a reason for hiding this comment

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

ULID?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe link to https://github.com/ulid/spec to disambiguate?

Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Thank you for taking this on @portertech!

@tigrannajaryan
Copy link
Member

Thanks @portertech !

@evan-bradley evan-bradley merged commit 8a4c306 into open-telemetry:main Oct 26, 2023
82 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 26, 2023
@portertech
Copy link
Contributor Author

Thank you all for your input and patience 🙇

sigilioso pushed a commit to carlossscastro/opentelemetry-collector-contrib that referenced this pull request Oct 27, 2023
See [design
document](https://docs.google.com/document/d/1KtH5atZQUs9Achbce6LiOaJxLbksNJenvgvyKLsJrkc/edit#heading=h.ioikt02qpy5f).

Depends on:

- [Implement ability for extensions to be notified about effective
configuration opentelemetry-collector#6596](open-telemetry/opentelemetry-collector#6596)
- [Make service.instance.id and other telemetry attributes available to
extensions opentelemetry-collector#6599](open-telemetry/opentelemetry-collector#6599)

Closes
open-telemetry#16618

---------

Signed-off-by: Sean Porter <[email protected]>
Co-authored-by: Daniel Jaglowski <[email protected]>
Co-authored-by: Tigran Najaryan <[email protected]>
jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this pull request Nov 12, 2023
See [design
document](https://docs.google.com/document/d/1KtH5atZQUs9Achbce6LiOaJxLbksNJenvgvyKLsJrkc/edit#heading=h.ioikt02qpy5f).

Depends on:

- [Implement ability for extensions to be notified about effective
configuration opentelemetry-collector#6596](open-telemetry/opentelemetry-collector#6596)
- [Make service.instance.id and other telemetry attributes available to
extensions opentelemetry-collector#6599](open-telemetry/opentelemetry-collector#6599)

Closes
open-telemetry#16618

---------

Signed-off-by: Sean Porter <[email protected]>
Co-authored-by: Daniel Jaglowski <[email protected]>
Co-authored-by: Tigran Najaryan <[email protected]>
RoryCrispin pushed a commit to ClickHouse/opentelemetry-collector-contrib that referenced this pull request Nov 24, 2023
See [design
document](https://docs.google.com/document/d/1KtH5atZQUs9Achbce6LiOaJxLbksNJenvgvyKLsJrkc/edit#heading=h.ioikt02qpy5f).

Depends on:

- [Implement ability for extensions to be notified about effective
configuration opentelemetry-collector#6596](open-telemetry/opentelemetry-collector#6596)
- [Make service.instance.id and other telemetry attributes available to
extensions opentelemetry-collector#6599](open-telemetry/opentelemetry-collector#6599)

Closes
open-telemetry#16618

---------

Signed-off-by: Sean Porter <[email protected]>
Co-authored-by: Daniel Jaglowski <[email protected]>
Co-authored-by: Tigran Najaryan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Component New component has been sponsored
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create opamp extension component
10 participants