opentelemetry-python icon indicating copy to clipboard operation
opentelemetry-python copied to clipboard

Add missing example requirements

Open pirgeo opened this issue 1 year ago • 5 comments

Description

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

Type of change

  • [x] 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:
  • [x] No.

Checklist:

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

pirgeo avatar Jun 27 '24 12:06 pirgeo

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.

pirgeo avatar Jun 27 '24 13:06 pirgeo

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 :sweat_smile: But let's see what maintainers think.

xrmx avatar Jun 27 '24 14:06 xrmx

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 :)

lzchen avatar Jun 27 '24 17:06 lzchen

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

ocelotl avatar Jun 27 '24 21:06 ocelotl

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

pirgeo avatar Jun 28 '24 13:06 pirgeo

@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.

lzchen avatar Jul 03 '24 17:07 lzchen

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.

pirgeo avatar Jul 05 '24 06:07 pirgeo

@pirgeo

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

lzchen avatar Jul 08 '24 16:07 lzchen

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

pirgeo avatar Jul 09 '24 07:07 pirgeo

@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 :)

lzchen avatar Jul 09 '24 16:07 lzchen

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

pirgeo avatar Jul 10 '24 14:07 pirgeo