open-telemetry / opentelemetry-sqlcommenter

SQLCommenter components for various languages
Apache License 2.0
28 stars 13 forks source link

Releasing python packages as opentelemetry SQLCommenter #20

Closed Thiyagu55 closed 2 years ago

Thiyagu55 commented 2 years ago
  1. Renamed python packages from google cloud to opentelemetry SQLCommenter
  2. Setup workflows which will publish the python package to testpypy and pypy
sjs994 commented 2 years ago

@aabmass @lonewolf3739 We need to add PYPI_API_TOKEN and TEST_PYPI_API_TOKEN to this repository to create a release from here. I am not aware of the process to get these tokens for opentelemetry account. Can you guys help here ?

srikanthccv commented 2 years ago

I still believe these packages are better merged to their corresponding instrumentations in opentelemetry-contrib-{js/py/rb/java}. If they are released as standalone packages sometimes they might even conflict with instrumentations (ex: both sqlcommenter and sqlalchemy-instrumentation use before_execute hook) or double patching may lead to strange issues. What do you think?

If we decide to continue releasing these packages we need to get the tokens shared from the opentelemetry-python maitainers team (I can request). I think they have something like 1password to share across the org maintainers.

sjs994 commented 2 years ago

I still believe these packages are better merged to their corresponding instrumentations in opentelemetry-contrib-{js/py/rb/java}. If they are released as standalone packages sometimes they might even conflict with instrumentations (ex: both sqlcommenter and sqlalchemy-instrumentation use before_execute hook) or double patching may lead to strange issues. What do you think?

If we decide to continue releasing these packages we need to get the tokens shared from the opentelemetry-python maitainers team (I can request). I think they have something like 1password to share across the org maintainers.

@lonewolf3739 I agree to the point that merging the changes to opentelemetry-contrib-* is the better way to do. But there are few tradeoffs and challenges we have to accept in that case:

  1. SQLCommenter would loose the ability to be installed in a standalone way.
  2. How much time it will take for opentelemetry-{py | js | ruby | java} maintainers to accept the merging into contrib repositories. At present we could not deprecate the google-cloud-sqlcommenter repository as there has been no release from here. Maintaining 2 repositories is getting difficult.

I was thinking can we do something like integrate opentelemetry-sqlcommenter library into opentelemetry-contrib-*. Like would it be feasible to call opentelemetry-sqlcommenter-python library methods from opentelemetry-contrib-python if some flag is enabled like use_sqlcommenter=<true|false> in open telemetry ?

srikanthccv commented 2 years ago

How much time it will take for opentelemetry-{py | js | ruby | java} maintainers to accept the merging into contrib repositories

Reviews happen reasonably quickly and packages get released at least once a month.

would it be feasible to call opentelemetry-sqlcommenter-python library methods from opentelemetry-contrib-python if some flag is enabled like use_sqlcommenter=<true|false> in open telemetry

Not sure I understand this completely, if the intent is to make instrumentations take dependency on opentelemetry-sqlcommenter-python, then it is techinically possible. That is similar to what we do with this. But maybe it would be easier to just update the instrumentation directly.

sjs994 commented 2 years ago

Reviews happen reasonably quickly and packages get released at least once a month.

I am not sure if the review is only needed by otel-python-maintainers. I had the impression that it requires a discussion with otel-specs SIG first for accepting the proposal to merge code into contrib repositories.

Not sure I understand this completely, if the intent is to make instrumentations take dependency on opentelemetry-sqlcommenter-python, then it is techinically possible. That is similar to what we do with this. But maybe it would be easier to just update the instrumentation directly.

Thank you, that was what i was asking. Like if there are no major cons, we could make the library work with/without opentelemetry. Any thoughts on this ? google-cloud-sqlcommenter-python has been an actively downloaded library from pypi. I was thinking if we can atleast do a release from here, we will go ahead and deprecate the code from google repository. In parallel we can start a discussion about merging the instrumentations into contrib repositories. If it is fine can you request keys from otel-python maintainers team ?

srikanthccv commented 2 years ago

I had the impression that it requires a discussion with otel-specs SIG first for accepting the proposal to merge code into contrib repositories.

didn't know that is the process.

we could make the library work with/without opentelemetry.

This is where I am conflicted. There is a decent info commenter adds to query other than trace context provided OTEL but trace context can be really useful to lookup and drill down to find the issues. Considering open-telemetry org is the new home for these packages I would expect the first class support for OTEL related stuff.

we will go ahead and deprecate the code from google repository. In parallel we can start a discussion about merging the instrumentations into contrib repositories

This sounds good to me. I will bring up the tokens matter with python maintainers team today.

sjs994 commented 2 years ago

This is where I am conflicted. There is a decent info commenter adds to query other than trace context provided OTEL but trace context can be really useful to lookup and drill down to find the issues. Considering open-telemetry org is the new home for these packages I would expect the first class support for OTEL related stuff.

trace context do provide very useful information as it is possible to join dots end to end. Example here. It makes sense that the repo being inside opentelemetry, it should have the first class support.

This sounds good to me. I will bring up the tokens matter with python maintainers team today.

Thank you very much!

srikanthccv commented 2 years ago

@sjs994 I brought up the idea of using the same account as opentelemetry-python/contrib. Only one of the maintainers was there in the meeting so there was no decision made. It would be ideal if you can join the next meeting when all the maintainers are present and we discuss about this. As there are holidays next meeting will be on first week of Jan. Does that sound good for you?

sjs994 commented 2 years ago

Thanks @lonewolf3739. Sure, will join the next meeting. It was good to have it quickly, but i expected there will be delay due to holidays. In the meantime, should we start a thread with otel-python-maintainers regarding what they think about moving sqlcommenter code into contrib repos ?

srikanthccv commented 2 years ago

We can do that but I don't think there will be any response as most of core team members are on leave starting from next week. We can still go ahead and start the discussion so that it sets some context and gives broader idea before we meet.

sjs994 commented 2 years ago

We discussed to make this PR into smaller ones. One for package rename and other for release actions.

Release actions would require the tokens for deployment, but package renames, we can review and merge before that.