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

Switch Python services to Elastic OTel distribution #41

Merged
merged 3 commits into from
Aug 20, 2024

Conversation

xrmx
Copy link
Member

@xrmx xrmx commented Aug 1, 2024

Changes

Switch recommendationservice and loadgenerator to Elastic OTel distribution

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
  • Appropriate Helm chart updates in the helm-charts

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.

@xrmx xrmx marked this pull request as ready for review August 1, 2024 13:31
@xrmx
Copy link
Member Author

xrmx commented Aug 1, 2024

This is completely untested but I don't expect surprises :)

@xrmx
Copy link
Member Author

xrmx commented Aug 1, 2024

  • Need to update loadgenerator too that for some reasons is using a very old distro

@xrmx xrmx changed the title recommendationservice: switch to elastic OTel distribution Switch to elastic OTel distribution Aug 2, 2024
Copy link
Collaborator

@rogercoll rogercoll left a comment

Choose a reason for hiding this comment

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

Thanks for adding this, just a small comment regarding if we should push these changes upstream too

src/loadgenerator/requirements.txt Outdated Show resolved Hide resolved
@xrmx
Copy link
Member Author

xrmx commented Aug 9, 2024 via email

@rogercoll
Copy link
Collaborator

rogercoll commented Aug 14, 2024

@xrmx Wdyt moving the version bump into the upstream repository? (we have automated syncs, and we try to avoid conflicts as much as possible)

I would just keep the Elastic's specific here elastic-opentelemetry==0.1.0

@xrmx
Copy link
Member Author

xrmx commented Aug 19, 2024

@xrmx Wdyt moving the version bump into the upstream repository? (we have automated syncs, and we try to avoid conflicts as much as possible)

I would just keep the Elastic's specific here elastic-opentelemetry==0.1.0

I've opened a PR here https://github.com/open-telemetry/opentelemetry-demo/pull/1705 but since the version of the sdk our distro depends on is not always synced with theirs or latest upstream, conflicts will happen anyway. Another source of conflicts is that the distro depends on more packages that the one they are installing.

.github/README.md Show resolved Hide resolved
.github/README.md Outdated Show resolved Hide resolved
@rogercoll
Copy link
Collaborator

@xrmx I was reviewing the conflicts in the loadgenerator requirements file and noticed that the distro package depends on more packages than the one being installed. Specifically, it seems that we are adding the opentelemetry-exporter-otlp, opentelemetry-exporter-otlp-proto-common, and opentelemetry-exporter-otlp-proto-http packages.

I was wondering if you could help me understand the purpose of these additions. Do these dependencies introduce additional attributes or signals to the loadgenerator service? I'm also curious about the reason behind including them in the requirements file.

@xrmx xrmx changed the title Switch to elastic OTel distribution Switch Python services to Elastic OTel distribution Aug 20, 2024
@xrmx
Copy link
Member Author

xrmx commented Aug 20, 2024

@xrmx I was reviewing the conflicts in the loadgenerator requirements file and noticed that the distro package depends on more packages than the one being installed. Specifically, it seems that we are adding the opentelemetry-exporter-otlp, opentelemetry-exporter-otlp-proto-common, and opentelemetry-exporter-otlp-proto-http packages.

I was wondering if you could help me understand the purpose of these additions. Do these dependencies introduce additional attributes or signals to the loadgenerator service? I'm also curious about the reason behind including them in the requirements file.

The additions come from our distro dependency on opentelemetry-exporter-otlp. I added that to avoid users to to have to install the otlp exporters explicitly. Currently our distro dependencies are the following:

dependencies = [
    "opentelemetry-api == 1.25.0",
    "opentelemetry-exporter-otlp == 1.25.0",
    "opentelemetry-instrumentation == 0.46b0",
    "opentelemetry-instrumentation-system-metrics == 0.46b0",
    "opentelemetry-semantic-conventions == 0.46b0",
    "opentelemetry-sdk == 1.25.0",
]

Which by chance matches what is in the upstream repository of this fork but I'll bump the dependency on 1.27.0 once it is out.

@rogercoll rogercoll merged commit 7bc364f into elastic:main Aug 20, 2024
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants