search

grafana / explore-profiles

Explore Profiles is a native Grafana application designed to integrate seamlessly with Pyroscope, the open-source continuous profiling platform, providing a smooth, query-less experience for browsing and analyzing profiling data.
GNU Affero General Public License v3.0
18 stars 1 forks source link

chore: accept pyroscope_git_session cookie name #151

Closed alsoba13 closed 1 month ago

alsoba13 commented 1 month ago

✨ Description

Related issue(s): https://github.com/grafana/pyroscope-squad/issues/171

Merge after datasources are able to forward new cookie name:

We are planning to rename the GitSession cookie to a more convenient pyroscope_git_session.

📖 Summary of the changes

🧪 How to test?

  1. Run this in local.
  2. Use a local pyroscope datasource as well, and check that GitHub Integration works as is (with the GitSession cookie name).
  3. Change backend to send pyroscope_git_session instead, and check that it works as well.
github-actions[bot] commented 1 month ago

Unit test coverage

Lines Statements Branches Functions
Coverage: 10%
 10.74% (416/3870) 8.32% (111/1333) 8.27% (101/1220)
alsoba13 commented 1 month ago

Looks good! I left some first pass comments, but I think this is a decent direction.

We should also make sure we're addressing what to do if we find a GitSession cookie already in the browser. We could:

  • leave it and use it as a fallback
  • rename it
  • delete it

No matter what is decided, let's be clear about how we plan on handling that case 👍

I changed the code to delete the old GitSession cookie in the presence of the new cookie (whenever we set pyroscope_git_session, GitSession gets deleted)

alsoba13 commented 1 month ago

@bryanhuhta I did the requested changes. I'm aware this solution is not perfect yet, but still wondering if payoffs deserve the effort taking into account that I was planning to revert everything related to multi-cookie support in this PR: https://github.com/grafana/explore-profiles/pull/152

How long do you expect to have this version running before merging the cleanup? I imagine myself merging the other PR before the changes in this PR hit production

alsoba13 commented 1 month ago

We decided to change the approach and go for a full migration in one step. We'll merge both frontend and backend with the new cookie name. We don't expect to have a relevant impact on this not being fully sync, as this feature is not very popular.

Yet, we will still delete old cookie names