v13: use cleaner sentry install

[tmorrell]

❤️ Thank you for your contribution!

Description

This changes the upgrade instructions to follow the sentry configuration instructions https://inveniordm.docs.cern.ch/operate/ops/logging/#configuring-sentry. I think it does exactly the same thing, just cleaner.

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

• I'm aware of the code of conduct.
• I've created logical separate commits and followed the commit message format.
• I've targeted the master branch.
• If this documentation change impacts the current release of InvenioRDM, I will backport it to the production branch following approval or indicate to a maintainer that it should be backported.

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

1. You license your contribution under the same terms as the current repository’s license.
2. You agree that you have the right to license your contribution under the current repository’s license.

[fenekku]

To be honest I am with you on that one, and made the same comment here #841 (comment) . I don't quite understand the reason given, but at the time we were just trying to get the docs out of the door for the release, and it had been validated by someone else, so I merged it in.

Maybe @ntarocco can provide more details when he is back (or someone else).

[tmorrell]

Yup, I'm happy to have this sit till there is a discussion.

My assumption was that pipenv would pick up the logging pin where it's defined elsewhere in Invenio.....but I haven't actually tested that. I also think telling people to update an invenio-logging version is easier than trying to get them to manage a sentry-sdk version.

And in any event we should make the instructions consistent across the documentation.

[ntarocco]

The reason was explained in the previous PR: we should not add invenio dependencies with versions ranges in the Pipfile (or in any user dependencies file), because we lose control of upgrades/changes.
As explained by Tom, this should work fine when the range is missing. In the past, we had issues with this depending if pip was installing first the package without a range or with a range. Hopefully, pipenv or uv has solved this. However, you can't test this with invenio-logging as we don't have other versions released. If you want to test it, you need to give it a try with other packages.

Given that the user still need to add a dependency with an extra, I don't see any diifference between writing invenio-logging with an extra, or sentry with an extra. When documented, users willing to use Sentry will simply copy/paste a line that we provide.

[fenekku]

Ah I see, Tom and I were more focused on the sentry-sdk = {extras = ["flask"] -> invenio-logging = {extras = ["sentry"]} and less so on the versions.

This now seems resolved @tmorrell and your changes good to go if you've tested them out somewhat.

[ntarocco]

Ah I see, Tom and I were more focused on the sentry-sdk = {extras = ["flask"] -> invenio-logging = {extras = ["sentry"]} and less so on the versions.

This now seems resolved @tmorrell and your changes good to go if you've tested them out somewhat.

I apologize if my previous message wasn't clear. I still believe that we shouldn't hardcode invenio dependencies in the Pipfile. If we ever remove or change the extra, we'll have to think about the instances out there, and document that change.
However, if Sentry makes a change, the user will need to update their configuration because of Sentry, not because of a change to invenio. I suspect that only a subset of Invenio users are using Sentry.

For these reasons, I don't see a clear benefit to this proposed change.

[fenekku]

Oh, I guess I didn't see in the end hahaha! ... and I still don't fully get it 😵‍💫 . I've investigated some more and here is what I have so far (let me know what is missing!):

1. We want to avoid telling users to add an invenio-<module> with version bounds in their requirements file, because it may prevent adoption of upgrades (the new major invenio-app-rdm might depend on an incompatible version of invenio-module for instance). The clashing invenio-<module> would have to be noticed and then bumped separately in the requirements file of the instance operator. Overall, that's a bad experience for InvenioRDM administrators and additional documentation burden for us.

My take: That makes sense to me if I understood correctly, and I agree with that recommendation.

2. We want to avoid telling users to even install invenio-logging[sentry] without version bounds specifically (and maybe this generalizes to other modules with extras cases?) because

   a) "If we ever remove or change the extra, we'll have to think about the instances out there, and document that change."
   b) "if Sentry makes a change, the user will need to update their configuration because of Sentry, not because of a change to invenio"
   c) there is something up with some of the tooling (there was a sidenote about this)

My take:

a) invenio-logging currently has sentry as an extra. So if we remove the extra or change it, we have to bump invenio-logging's major version and document the change since it breaks its public interface. In other words, we have to do this, wether or not we recommend people put invenio-logging[sentry] in their requirements file. And spoiler for c), we didn't do a major bump for sentry_sdk -> sentry . That change was done as patch from invenio-logging 2.1.4 to invenio-logging 2.1.5 which causes problem.

b) The sentry logging capabilities of an instance are provided via invenio-logging (https://github.com/inveniosoftware/invenio-logging/blob/v2.1.4/invenio_logging/sentry.py). If Sentry makes a change, that change is either breaking or non-breaking to invenio-logging code. But not using the extra invenio-logging[sentry] will not bring about an error in dependency resolution (see next paragraph though), it will likely bring about the error at runtime. If we let sentry-sdk with version bounds be placed in the requirements file, then when invenio-logging starts actually depending on a different range of version for sentry-sdk we are back to case 1) .

The dependency resolution with optional dependencies is perhaps the crux of the problem though. As in, it wouldn't even work properly anyway which is why recommending to put sentry-sdk in the requirements file directly was recommended.

c) There is indeed something up with the dependency tooling:

With pipenv, version 2024.4.1 and a Pipfile as follows:

[packages]
invenio-app-rdm = {extras = ["opensearch2"], version = "~=12.0.0"}
invenio-logging = {extras = ["sentry_sdk"]}

The locking result is

invenio-logging 2.1.5
and NO sentry-related module installed

That is unexpected. I would have expected :

invenio-logging 2.1.4
and sentry -related module installed.

It's the same thing for uv 0.7.19 actually.

In both cases, only a warning is issued when locking, and installing is let through. So perhaps there are more issues with dependency resolution that explains the recommendation.

I already went over my timebox for this, so I will stop there and only pick it up after Nico's holidays 😆.