-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
OpAMP Agent Extension #16594
Conversation
part of open-telemetry#16462 Signed-off-by: Sean Porter <portertech@gmail.com>
Signed-off-by: Sean Porter <portertech@gmail.com>
Signed-off-by: Sean Porter <portertech@gmail.com>
Signed-off-by: Sean Porter <portertech@gmail.com>
Signed-off-by: Sean Porter <portertech@gmail.com>
Signed-off-by: Sean Porter <portertech@gmail.com>
@tigrannajaryan @dmitryax am I on the right track here? Still very early/raw 😅 Based on the opamp-go examples and my configmap provider POC. |
Signed-off-by: Sean Porter <portertech@gmail.com>
Foresight Summary
View More Details✅ check-links workflow has finished in 51 seconds (1 minute 9 seconds less than
|
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 |
*You can configure Foresight comments in your organization settings page.
There was a problem hiding this 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!
extension/opampextension/config.go
Outdated
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"` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Signed-off-by: Sean Porter <portertech@gmail.com>
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
@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. |
@mwear I was in a holding pattern for a few other changes. I do want to dust it off in the near future. |
Signed-off-by: Sean Porter <portertech@gmail.com>
Signed-off-by: Sean Porter <portertech@gmail.com>
Signed-off-by: Sean Porter <portertech@gmail.com>
Signed-off-by: Sean Porter <portertech@gmail.com>
Signed-off-by: Sean Porter <portertech@gmail.com>
Signed-off-by: Sean Porter <portertech@gmail.com>
There was a problem hiding this 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.
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
Signed-off-by: Sean Porter <portertech@gmail.com>
Signed-off-by: Sean Porter <portertech@gmail.com>
- `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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ULID?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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!
Thanks @portertech ! |
Thank you all for your input and patience 🙇 |
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 <portertech@gmail.com> Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com> Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
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 <portertech@gmail.com> Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com> Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
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 <portertech@gmail.com> Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com> Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
See design document.
Depends on:
Closes #16618