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

add advance conf for otel-collector #212

Merged
merged 13 commits into from
Aug 4, 2022
Merged

add advance conf for otel-collector #212

merged 13 commits into from
Aug 4, 2022

Conversation

Frapschen
Copy link
Contributor

@Frapschen Frapschen commented Jul 16, 2022 •
edited
Loading

Fixes #174

I think the spanmetrics processor is one of a good demo for advance ability of otel-collector.

Changes

  • add a spanmetrics processor in otel-collector
  • use otel/opentelemetry-collector-contrib:0.52.0 image to support advance ability
  • use released images in docker-compose

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #

@Frapschen Frapschen requested a review from a team July 16, 2022 05:48
wph95
wph95 previously requested changes Jul 20, 2022
View reviewed changes
Copy link
Member

@wph95 wph95 left a comment

Choose a reason for hiding this comment

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

I saw you change some file

  • add jaeger-ui.json
  • change otel-collector config
  • change docker compose image name logic

One pr per feature, which will make the review more efficient.

@Frapschen
Copy link
Contributor Author

Frapschen commented Jul 22, 2022 •
edited
Loading

I saw you change some file

  • add jaeger-ui.json
  • change otel-collector config
  • change docker compose image name logic

One pr per feature, which will make the review more efficient.

sorry, I have recovered the unrelated change

@Frapschen Frapschen requested a review from wph95 July 22, 2022 07:44
puckpuck
puckpuck requested changes Jul 27, 2022
View reviewed changes
Copy link
Contributor

@puckpuck puckpuck left a comment

Choose a reason for hiding this comment

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

Some small nitpick changes on the Otel Config side, but also would want to see all the Jaeger changes removed from this PR.

src/otelcollector/otelcol-config.yml Outdated Show resolved Hide resolved
@Frapschen
Copy link
Contributor Author

Some small nitpick changes on the Otel Config side, but also would want to see all the Jaeger changes removed from this PR.

Jaeger have a monitor page to show the span metrics, I think we can ues it.

@cartersocha
Copy link
Contributor

Some small nitpick changes on the Otel Config side, but also would want to see all the Jaeger changes removed from this PR.

Jaeger have a monitor page to show the span metrics, I think we can ues it.

Our preference / design is native OTel metrics only with maybe Prometheus if needed consumed in grafana. We’re not planning on using jaeger for metrics in the main demo & by default we’d like it to be off. Users are welcome to enable it on their own if they choose

@Frapschen Frapschen requested a review from puckpuck August 1, 2022 08:24
julianocosta89
julianocosta89 approved these changes Aug 3, 2022
View reviewed changes
Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

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

Just a version bump, but looks good to me!

docker-compose.yml Outdated Show resolved Hide resolved
@cartersocha cartersocha dismissed wph95’s stale review August 3, 2022 16:59

change was made as requested

@cartersocha
Copy link
Contributor

@Frapschen please put a changelog entry for this & we'll merge!

wph95
wph95 approved these changes Aug 4, 2022
View reviewed changes
Copy link
Member

@wph95 wph95 left a comment

Choose a reason for hiding this comment

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

LGTM

CHANGELOG.md Outdated Show resolved Hide resolved
@puckpuck puckpuck merged commit 3ae87ca into open-telemetry:main Aug 4, 2022
@Frapschen Frapschen deleted the add_advance_demo branch August 4, 2022 08:23
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
@Frapschen @cijothomas
@julianocosta89 @cartersocha @wph95
add advance conf for otel-collector (open-telemetry#212)
c136193 
* add advance conf for otel-collector

* undo not relate change

* Apply suggestions from code review

Co-authored-by: Cijo Thomas <[email protected]>

* apply reviewer's suggestion

* apply reviewer's suggestion

* Apply suggestions from code review

Co-authored-by: Juliano Costa <[email protected]>

* add changelog entry.

* fix markdown

* Apply suggestions from code review

Co-authored-by: JustWPH <[email protected]>

Co-authored-by: Cijo Thomas <[email protected]>
Co-authored-by: Juliano Costa <[email protected]>
Co-authored-by: Carter Socha <[email protected]>
Co-authored-by: JustWPH <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@puckpuck puckpuck puckpuck approved these changes

@wph95 wph95 wph95 approved these changes

@mic-max mic-max mic-max approved these changes

@cijothomas cijothomas Awaiting requested review from cijothomas

@julianocosta89 julianocosta89 Awaiting requested review from julianocosta89

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add some "demos" for collector components
7 participants
@Frapschen @cartersocha @puckpuck @wph95 @cijothomas @mic-max @julianocosta89