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

Add missing example requirements #4000

Conversation

pirgeo
Copy link
Member

@pirgeo pirgeo commented Jun 27, 2024

Description

Adds missing requirements.txt for one example (https://github.com/open-telemetry/opentelemetry-python/tree/main/docs/examples/metrics/instruments).

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

pip install -r requirements.txt
python example.py                      # this is already part of the docs

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@pirgeo pirgeo requested a review from a team as a code owner June 27, 2024 12:48
Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

I would add a pip install opentelemetry-api opentelemetry-sdk opentelemetry-exporter-otlp to the README instead so we don't add another place where we need to bump the versions

@pirgeo
Copy link
Member Author

pirgeo commented Jun 27, 2024

I tried to align with the other examples, which all have requirements.txt files. I don't have a preference for either, so I am happy to change it to simply adding it to the readme.

@xrmx
Copy link
Contributor

xrmx commented Jun 27, 2024

I tried to align with the other examples, which all have requirements.txt files. I don't have a preference for either, so I am happy to change it to simply adding it to the readme.

Given the status of these requirements I think I have a stronger opinion now 😅 But let's see what maintainers think.

@xrmx xrmx added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jun 27, 2024
Copy link
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

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

It seems we are not bumping the packages versions on requirements.txt for those examples, this explain why we have some requirements.txt file with opentelemetry-sdk=1.15.0 .

@lzchen
Copy link
Contributor

lzchen commented Jun 27, 2024

I tried to align with the other examples, which all have requirements.txt files. I don't have a preference for either, so I am happy to change it to simply adding it to the readme.

Given the status of these requirements I think I have a stronger opinion now 😅 But let's see what maintainers think.

+1 for this, less maintenence the better :)

@ocelotl
Copy link
Contributor

ocelotl commented Jun 27, 2024

I suggest we keep the requirements.txt file, just remove the pinned versions.

@pirgeo
Copy link
Member Author

pirgeo commented Jun 28, 2024

I updated it to use >= instead of ==. Please let me know if you think this works, otherwise I'll just drop the versions completely!

@lzchen
Copy link
Contributor

lzchen commented Jul 3, 2024

@ocelotl

I suggest we keep the requirements.txt file, just remove the pinned versions.

We don't requirements.txt file for the examples. Do we really want to start adding them for each example? This seems like more maintenance overhead.

@pirgeo
Copy link
Member Author

pirgeo commented Jul 5, 2024

I was looking at the other examples in that folder (https://github.com/open-telemetry/opentelemetry-python/tree/main/docs/examples/metrics/views, https://github.com/open-telemetry/opentelemetry-python/tree/main/docs/examples/metrics/reader, https://github.com/open-telemetry/opentelemetry-python/tree/main/docs/examples/metrics/prometheus-grafana) which all have a requirements.txt. Only this example (at least in the https://github.com/open-telemetry/opentelemetry-python/tree/main/docs/examples/metrics folder) doesn't have one.
I am fine with not adding it, but having the requirements.txt definitely makes it easier to set up and try out the examples, since you don't have to guess which packages to install first. This might be especially helpful if you haven't worked with OTel before and these examples are your first touchpoint with the project.

@lzchen
Copy link
Contributor

lzchen commented Jul 8, 2024

@pirgeo

Oh apologies, was referring to the other folders. If every other example in metrics has requirements.txt i don't mind adding this.

@pirgeo
Copy link
Member Author

pirgeo commented Jul 9, 2024

Changed it to use ~= as per @lzchen's suggestion.

@lzchen
Copy link
Contributor

lzchen commented Jul 9, 2024

@pirgeo

Needs another rebase. Might want to change the permissions of the pr to allow maintainer edits so we can rebase for you next time :)

@pirgeo pirgeo force-pushed the add-missing-example-requirements branch from 645cf39 to 17a4426 Compare July 10, 2024 14:34
@pirgeo
Copy link
Member Author

pirgeo commented Jul 10, 2024

Ah, the organization fork does not have that automatically. I enabled the maintainer modification access now and rebased it

@lzchen lzchen merged commit 61ea97d into open-telemetry:main Jul 11, 2024
269 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants