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

chore: permanently deprecate opentelemetry-browser-extension-autoinjection #2285

Merged
merged 5 commits into from
Jun 27, 2024

Conversation

svrnm
Copy link
Member

@svrnm svrnm commented Jun 19, 2024

As discussed in #1152 (comment) this PR will permanently deprecate the OpenTelemetry Browser Extension in this repository. While this was not a lighthearted decision, I think it is the right thing to do, since it

In this PR I make sure that it is clear that this is permanent, but that there are alternatives and that they can be found via the Registry (and people can add more if they want to)

@SimenB I appreciate the help you provided on fixing and debugging the underlying issue with webpack4&webpack5 and I apologize that this didn't land

Note, that I kicked off a conversation with @tbrockman about the extension at https://github.com/tbrockman/opentelemetry-browser-extension and shared some if the ideas I had for the extension. I also played around with this extension and it is much feature-richer and stable than what I created, so at the end this is a happy ending!

Thank you all

@svrnm svrnm requested a review from a team June 19, 2024 14:57
Copy link

codecov bot commented Jun 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.30%. Comparing base (dfb2dff) to head (2184589).
Report is 177 commits behind head on main.

Current head 2184589 differs from pull request most recent head 029a5f0

Please upload reports for the commit 029a5f0 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2285      +/-   ##
==========================================
- Coverage   90.97%   90.30%   -0.68%     
==========================================
  Files         146      147       +1     
  Lines        7492     7263     -229     
  Branches     1502     1509       +7     
==========================================
- Hits         6816     6559     -257     
- Misses        676      704      +28     

see 57 files with indirect coverage changes

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Thank you for the update.
Happy to hear that there's a third-party replacement for the package. 🙂

@blumamir
Copy link
Member

Thanks @svrnm

@open-telemetry/android-maintainers do you think we should keep the deprecated package in this repo?

@pichlermarc
Copy link
Member

pichlermarc commented Jun 20, 2024

@blumamir I think we should keep the README.md to tell people where to go to find the replacement. 🙂

@LikeTheSalad
Copy link

Thanks @svrnm

@open-telemetry/android-maintainers do you think we should keep the deprecated package in this repo?

I'm a bit confused, is it related to an android package?

@pichlermarc
Copy link
Member

I'm a bit confused, is it related to an android package?

@blumamir must've pinged the wrong group. I think he meant @open-telemetry/javascript-maintainers 🙂

@blumamir
Copy link
Member

I'm a bit confused, is it related to an android package?

@blumamir must've pinged the wrong group. I think he meant @open-telemetry/javascript-maintainers 🙂

yes, my bad. sorry about that :)

@svrnm
Copy link
Member Author

svrnm commented Jun 20, 2024

@blumamir I think we should keep the README.md to tell people where to go to find the replacement. 🙂

I am OK with both, but I would prefer to keep it archived for that same reason

@JamieDanielson
Copy link
Member

@blumamir I think we should keep the README.md to tell people where to go to find the replacement. 🙂

I am OK with both, but I would prefer to keep it archived for that same reason

I see a bit of "why not both" here. If we keep the README, we can also include a link to the code (say from a previous tag) and remove the code from main. Another alternative is to - whether now or later - create a separate markdown file linked from the main README that includes an index (list) of deprecated/archived projects like this one, i.e. deprecated.md

@svrnm
Copy link
Member Author

svrnm commented Jun 20, 2024

I see a bit of "why not both" here. If we keep the README, we can also include a link to the code (say from a previous tag) and remove the code from main. Another alternative is to - whether now or later - create a separate markdown file linked from the main README that includes an index (list) of deprecated/archived projects like this one, i.e. deprecated.md

I like that (doing it incrementally + having a deprecated.md that points to the commit with the code).

I know that JS SIG is not the only one looking into that, I wonder how other SIGs are handling that?

@blumamir
Copy link
Member

I support removing the code (which one can also find in git by checking out to an early commit) and leaving just a note in the README to communicate the status so that it won't go away for some reasonable time.

The code pops up in IDE searches and may still promote old practices which I don't see any good reason to keep anymore.

@svrnm
Copy link
Member Author

svrnm commented Jun 24, 2024

@blumamir @pichlermarc @JamieDanielson whatever you prefer doing I can implement via this PR, or feel free to go ahead and raise one separately to get this fixed

@pichlermarc
Copy link
Member

@blumamir @pichlermarc @JamieDanielson whatever you prefer doing I can implement via this PR, or feel free to go ahead and raise one separately to get this fixed

Thanks 👍
Let's merge this PR and then we can still follow up on removing the code. It being here for now does not hurt anyone 🙂

@pichlermarc pichlermarc merged commit 688a92d into open-telemetry:main Jun 27, 2024
14 checks passed
@svrnm svrnm deleted the deprecate-my-browser-extension branch June 27, 2024 09:02
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.

5 participants