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

[extension/opampextension] reports effective config #29277

Merged
merged 13 commits into from
Nov 30, 2023
Merged

[extension/opampextension] reports effective config #29277

merged 13 commits into from
Nov 30, 2023

Conversation

haoqixu
Copy link
Member

@haoqixu haoqixu commented Nov 14, 2023 •
edited

Link to tracking Issue: resolve #27293

Testing:
unittest
manual test with opamp-go example:

  • collector version: modified otelcontribcol including opampextension
  • collector config:
receivers:
  # otlp:
  #   protocols:
  #     grpc:
  filelog:
    include:
      - /tmp/xxxxx.log

exporters:
  debug:
    #verbosity: debug

extensions:
  opamp:
    server:
      ws:
        endpoint: ws://127.0.0.1:4320/v1/opamp
        tls:
          insecure_skip_verify: true

service:
  pipelines:
    logs:
      receivers: [filelog]
      exporters: [debug]
  telemetry:
    logs:
      level: info
  extensions: [opamp]
  • server ui: Screenshot 2023-11-28 at 22-14-59 OpAMP Server

Documentation: README.md

@haoqixu haoqixu force-pushed the feat-27293-reports-effective-config branch 4 times, most recently from 0c32485 to d7c499d Compare November 17, 2023 16:52
@haoqixu haoqixu marked this pull request as ready for review November 17, 2023 17:23
JaredTan95

This comment was marked as abuse.

Sign in to view

@JaredTan95 JaredTan95 self-requested a review November 26, 2023 09:05
@JaredTan95
Copy link
Member

JaredTan95 commented Nov 26, 2023 •
edited

Sorry, I planned to modify the review comment again, but I chose the wrong hide comments option(hidden as abuse 😅.)~

@fatsheep9146
Copy link
Contributor

@evan-bradley @tigrannajaryan do you think this should be implemented as feature gate first?

tigrannajaryan
tigrannajaryan previously approved these changes Nov 27, 2023
View reviewed changes
extension/opampextension/config.go Show resolved Hide resolved
extension/opampextension/opamp_agent.go Outdated Show resolved Hide resolved
extension/opampextension/opamp_agent.go Show resolved Hide resolved
@tigrannajaryan tigrannajaryan dismissed their stale review November 27, 2023 17:44

Approved accidentally

@tigrannajaryan
Copy link
Member

manual test with opamp-go example

Can you please attach a screenshot showing what it looks like in the server UI?

@tigrannajaryan
Copy link
Member

@evan-bradley @tigrannajaryan do you think this should be implemented as feature gate first?

I don't think it is needed. The opamp extension is in "development" state and there is no stability expectations.

@haoqixu haoqixu force-pushed the feat-27293-reports-effective-config branch from 3508ada to 0a86f4e Compare November 28, 2023 10:38
@haoqixu haoqixu mentioned this pull request Nov 28, 2023
Merged
@haoqixu
Copy link
Member Author

haoqixu commented Nov 28, 2023 •
edited

manual test with opamp-go example

Can you please attach a screenshot showing what it looks like in the server UI?

👌 I edited the PR to include the collector config and a screenshot of the server UI.

tigrannajaryan
tigrannajaryan approved these changes Nov 28, 2023
View reviewed changes
extension/opampextension/opamp_agent.go Outdated Show resolved Hide resolved
@haoqixu haoqixu force-pushed the feat-27293-reports-effective-config branch from c79ad44 to 1fe3061 Compare November 28, 2023 16:24
dmitryax pushed a commit to open-telemetry/opentelemetry-collector that referenced this pull request Nov 28, 2023
@haoqixu @evan-bradley
[chore][extension] update NotifyConfig contract (#9005)
7d2cb74 
**Description:**
The collector implementation makes a copy of effective configuration
before calling `NotifyConfig` so it's safe for the config watcher to
save the pointer and use it later. It is resonable to promise this in
the API contract.

**Link to tracking Issue:**
open-telemetry/opentelemetry-collector-contrib#29277 (comment)

---------

Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
@evan-bradley evan-bradley merged commit 16760c7 into open-telemetry:main Nov 30, 2023
83 checks passed
@github-actions github-actions bot added this to the next release milestone Nov 30, 2023
@haoqixu haoqixu mentioned this pull request Dec 8, 2023
Closed
pantuza pushed a commit to pantuza/opentelemetry-collector that referenced this pull request Dec 8, 2023
@haoqixu @evan-bradley @pantuza
[chore][extension] update NotifyConfig contract (open-telemetry#9005)
2efc352 
**Description:**
The collector implementation makes a copy of effective configuration
before calling `NotifyConfig` so it's safe for the config watcher to
save the pointer and use it later. It is resonable to promise this in
the API contract.

**Link to tracking Issue:**
open-telemetry/opentelemetry-collector-contrib#29277 (comment)

---------

Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
@evan-bradley evan-bradley mentioned this pull request Apr 16, 2024
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fatsheep9146 fatsheep9146 fatsheep9146 left review comments

@JaredTan95 JaredTan95 JaredTan95 approved these changes

@portertech portertech portertech approved these changes

@evan-bradley evan-bradley evan-bradley approved these changes

@atoulme atoulme Awaiting requested review from atoulme atoulme is a code owner

@tigrannajaryan tigrannajaryan Awaiting requested review from tigrannajaryan tigrannajaryan is a code owner

Assignees

@fatsheep9146 fatsheep9146

Labels
cmd/opampsupervisor extension/opamp
Projects
None yet
Milestone
v0.90.1
Development

Successfully merging this pull request may close these issues.

[extension/opampextension] Subscribe to be notified of Collector's effective config
6 participants
@haoqixu @JaredTan95 @fatsheep9146 @tigrannajaryan @portertech @evan-bradley