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

Jaeger-SPM-Config #655

Merged
merged 9 commits into from
Dec 25, 2022
Merged

Conversation

devrimdemiroz
Copy link
Contributor

@devrimdemiroz devrimdemiroz commented Dec 21, 2022

Changes

prometheus.server-url parameter and environment variable - METRICS_STORAGE_TYPE=prometheus added. So that Jaeger SPM window now shows spanmetrics.

It is not a crucial need as Jager would already be datasourced by Grafana. Nevertheless, it is nice to have.

Merge Requirements

For new features contributions please make sure you have completed the following
essential items:

  • CHANGELOG.md updated to document new feature additions
  • Appropriate documentation updates in the docs folder

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

--prometheus.server-url parameter and environment variable METRICS_STORAGE_TYPE=prometheus added to make Jaeger SPM page working.
prometheus.server-url parameter and environment variable
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 21, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@devrimdemiroz devrimdemiroz marked this pull request as ready for review December 21, 2022 19:26
@devrimdemiroz devrimdemiroz requested a review from a team December 21, 2022 19:26
Copy link
Member

@mviitane mviitane left a comment

Choose a reason for hiding this comment

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

Looks good and seems to work fine!
image

@devrimdemiroz, can you still fix the yamllint problem (add spaces after commas)?

docker-compose.yml Outdated Show resolved Hide resolved
@puckpuck puckpuck added the helm-update-required Requires an update to the Helm chart when released label Dec 24, 2022
commited to test in local. will be resolving once it works.

Co-authored-by: Pierre Tessier <pierre@pierretessier.com>
Copy link
Contributor Author

@devrimdemiroz devrimdemiroz left a comment

Choose a reason for hiding this comment

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

works fine. Thanks.

@puckpuck
Copy link
Contributor

Please add a changelog entry, and cleanup the failed checks.

@julianocosta89 julianocosta89 merged commit b9b5cd9 into open-telemetry:main Dec 25, 2022
@devrimdemiroz devrimdemiroz deleted the patch-1 branch December 25, 2022 12:49
@puckpuck
Copy link
Contributor

puckpuck commented Jan 4, 2023

So to make this change work with the Helm chart will require an update to the Jaeger helm chart to accept passing in templated variables properly.

Managing dependencies is fun 🤪

jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
* prometheus.server-url parameter

--prometheus.server-url parameter and environment variable METRICS_STORAGE_TYPE=prometheus added to make Jaeger SPM page working.

* prometheus.server-url

prometheus.server-url parameter and environment variable

* Update docker-compose.yml

commited to test in local. will be resolving once it works.

Co-authored-by: Pierre Tessier <pierre@pierretessier.com>

* lint typo correctin

* Update CHANGELOG.md

* Add Jaeger-SPM-Config
([open-telemetry#655](open-telemetry#655))

* remove extra blank space

Co-authored-by: Pierre Tessier <pierre@pierretessier.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helm-update-required Requires an update to the Helm chart when released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants