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

Add support for Events Stats queries #225

Merged
merged 7 commits into from
Apr 17, 2024

Conversation

oblador
Copy link
Contributor

@oblador oblador commented Apr 2, 2024

This PR adds support for Events Stats queries, which is the type that drives the Sentry Dashboards, which gives you a time series result unlike the regular Events.

Since the JSON result is polymorphic and doesn't really have any predetermined field names (except for meta), the unmarshalling of the data is not incredibly elegant. I'm new to go so maybe there's a cleaner way, but from my googling there wasn't an obvious better way.

One difference from the Stats query is that I don't provide an Interval field, but instead derive that from the built in query options which IMHO feels more Grafana native and more user friendly.

The UX of the query editor could be improved by providing a click based builder like the Sentry Dashboard, but I think this is a good start.

Closes #33

Screenshot 2024-04-02 at 11 38 16

@oblador oblador requested a review from a team as a code owner April 2, 2024 09:49
@oblador oblador requested review from asimpson, bossinc, aangelisc and alyssabull and removed request for a team April 2, 2024 09:49
@CLAassistant
Copy link

CLAassistant commented Apr 2, 2024

CLA assistant check
All committers have signed the CLA.

@oblador oblador force-pushed the events-stats-query branch from ac38915 to edda72a Compare April 2, 2024 11:37
Copy link
Contributor

@bossinc bossinc 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 this contribution! Event stats queries work great 😃

Since the JSON result is polymorphic and doesn't really have any predetermined field names (except for meta), the unmarshalling of the data is not incredibly elegant. I'm new to go so maybe there's a cleaner way, but from my googling there wasn't an obvious better way.

This is similar to how we handle undefined data in other plugins. It's always kind of difficult.

One difference from the Stats query is that I don't provide an Interval field, but instead derive that from the built in query options which IMHO feels more Grafana native and more user friendly.

Agreed.

The UX of the query editor could be improved by providing a click based builder like the Sentry Dashboard, but I think this is a good start.

Good callout but like you said this is absolutely a good start.

I have a few minor nits, but everything else looks great. I am not as familiar with this code as @asimpson. He will be back in the office next week, and with his approval, we can merge this in. Thank you again!

pkg/plugin/handlers_query_test.go Show resolved Hide resolved
pkg/plugin/framer.go Outdated Show resolved Hide resolved
@oblador
Copy link
Contributor Author

oblador commented Apr 15, 2024

@asimpson Eager to start using this datasource 😄 Would be greatly appreciated if you would have a look at this one! Thanks 🙇

Copy link
Contributor

@adamyeats adamyeats left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution here @oblador! Some comments from me below, once resolved we'll be happy to go ahead and merge this one 👍

pkg/plugin/framer.go Outdated Show resolved Hide resolved
src/components/query-editor/ScopePicker.tsx Outdated Show resolved Hide resolved
pkg/plugin/framer.go Outdated Show resolved Hide resolved
pkg/plugin/framer.go Outdated Show resolved Hide resolved
@oblador
Copy link
Contributor Author

oblador commented Apr 16, 2024

Thanks for the review @adamyeats! I think I've addressed all points now 👍

@adamyeats
Copy link
Contributor

Thanks for being so on top of this @oblador ! Looks like there's some test failures here, would you be able to take a look? I've attached log output here in case you're unable to access the link to Drone.

logs_grafana_sentry-datasource_605_1_7.log

@oblador
Copy link
Contributor Author

oblador commented Apr 17, 2024

@adamyeats I'm not so familiar with go tool chain, so I can't say exactly why but those failures seems to be cache related. If you do go clean -testcache does that fix it? I think at some point I had to comment assertion lines and uncomment again to bypass the cache. Strange and annoying, but my latest change didn't change this behavior and they pass locally for me. Doesn't these tests run in the CI?

@adamyeats
Copy link
Contributor

adamyeats commented Apr 17, 2024

@oblador The output I attached to my previous comment was from CI, you can see the output here. This doesn't look cache related to me, as our pipeline doesn't cache anything test related. This seems to be an intermittent issue that I'm also able to reproduce locally, even after running go clean -testcache, with go test ./pkg/... -count 10.

A colleague has pointed out to me that this might be because the ordering of fields isn't guaranteed. If we want it to be guaranteed then the fields would have to be sorted. There is an example of this in another datasource here. Apologies for not catching this in my review and thank you for your continued work on this!

@oblador
Copy link
Contributor Author

oblador commented Apr 17, 2024

@adamyeats Aah, thanks for the insight! I refactored the tests to use Contains assertions instead of assuming deterministic behaviour (pretty wild to me that it isn't!).

Copy link
Contributor

@adamyeats adamyeats left a comment

Choose a reason for hiding this comment

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

@oblador Nice! Very happy to merge this in, thanks for your hard work. I'll talk to the team and aim to get a new release out with this included ASAP.

@adamyeats adamyeats enabled auto-merge (squash) April 17, 2024 13:24
@adamyeats adamyeats merged commit 4b43c90 into grafana:main Apr 17, 2024
4 of 7 checks passed
@oblador
Copy link
Contributor Author

oblador commented Apr 24, 2024

What did the release team say? Anything blocking a release? 😊

@edoardopirovano
Copy link

What did the release team say? Anything blocking a release? 😊

I'd be keen to start using this too so would be great if it could be released! 🙂

@oblador
Copy link
Contributor Author

oblador commented May 7, 2024

@adamyeats Apologies for being pushy, but I would really need to get this released soon. Would you mind reaching out to the team? I trust the release is automated anyways. Thanks!

@adamyeats
Copy link
Contributor

@oblador Apologies for the delay here! I have published 1.7.0 this afternoon with your changes included. Let us know if there are any problems and thank you for your patience here!

@oblador
Copy link
Contributor Author

oblador commented May 7, 2024

Awesome, thank you!

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.

Performance transactions in grafana
5 participants