Prometheus schematization #63878

itsmylife · 2023-02-28T15:36:09Z

What is this feature?

This is an example PR to illustrate schematizing the Prometheus datasource plugin. Probably It won't be merged.
The guidance https://docs.google.com/document/d/1RRPGeD7DQ7wot0Pwnc4UZolXrZYip0ViqOozl9zbBpE/edit#

Which issue(s) does this PR fix?:

Closes #63874

Special notes for your reviewer:
This PR is an up-to-date version of this PR. I closed that one because it was behind the main and getting hard to merge it because of the conflicts.

# Conflicts: # public/app/plugins/datasource/prometheus/types.ts

ryantxu · 2023-03-01T05:24:47Z

public/app/plugins/datasource/prometheus/dataquery.cue

+						interval?:       string
+						intervalMs?:     int64


Are these set in the UI, or as part of the request? If we can not set them from the UI, lets keep them out of this schema.

while i have a feeling we may find exceptions to this rule (such that we need to find a better rule), i think it's the right one at least for now

I made the changes related to this. See: d4db739

ryantxu · 2023-03-01T05:25:29Z

public/app/plugins/datasource/prometheus/dataquery.cue

+					{
+						common.DataQuery
+						expr:            string
+						format?:         string


What are the valid values? I think 'timeseries' and 'table', but would be nice to have that explicit :)

Agree, if there's a set of acceptable values here, they should be explicitly declared.

Could make it a reference to another type, as with #QueryEditorMode, if that type needs to be used elsewhere in TS

I will check the possible values and introduce a change.

See a0bd621

sdboyer

i don't know the specifics of the prom datasource, but overall this looks pretty close! i think i'd just simplify a bit by removing pointers and some possibly-unnecessary fields from the schema as @ryantxu noted

pkg/tsdb/prometheus/models/query.go

sdboyer · 2023-03-07T11:52:51Z

public/app/plugins/datasource/prometheus/dataquery.cue

+					{
+						common.DataQuery
+						expr:            string
+						format?:         string


Agree, if there's a set of acceptable values here, they should be explicitly declared.

Could make it a reference to another type, as with #QueryEditorMode, if that type needs to be used elsewhere in TS

Co-authored-by: sam boyer <sdboyer@grafana.com>

public/app/plugins/datasource/prometheus/dataquery.gen.ts

ryantxu · 2023-03-08T20:21:34Z

docs/sources/developers/kinds/composable/prometheusdataquery/schema-reference.md

+| `format`     | string  | No       | Possible values are: `time_series`, `table`, `heatmap`.                                                                                                                                                                                                                                                |
+| `hide`       | boolean | No       | *(Inherited from [DataQuery](#dataquery))*<br/>true if query is disabled (ie should not be returned to the dashboard)                                                                                                                                                                                  |
+| `instant`    | boolean | No       |                                                                                                                                                                                                                                                                                                        |
+| `key`        | string  | No       | *(Inherited from [DataQuery](#dataquery))*<br/>Unique, guid like, string used in explore mode                                                                                                                                                                                                          |


I was surprised to see "key" here... but then see that is not added in this PR

#64467

ryantxu · 2023-03-08T20:25:19Z

public/app/plugins/datasource/prometheus/dataquery.cue

+			{
+				schemas: [
+					{
+						common.DataQuery


Any effort on comments here will be HUGELY helpful. The text+values here will drive all the docs and API generation, so if we can try to explain the parameters here (and agree/confirm they are accurate!) that would be awesome.

public/app/plugins/datasource/prometheus/dataquery.cue

ryantxu · 2023-03-08T20:28:06Z

public/app/plugins/datasource/prometheus/dataquery.cue

+						instant?:    bool
+						range?:      bool


Can both range and instant be true?

they can't, and we can natively constrain them in CUE to indicate that invariant, but at the moment cuetsy evaluates the operation and converts the TS output to just false - see grafana/cuetsy#75

Yes, it can be. In query options there is a type option field:

oh! i totally misunderstood this on my first pass through it, then :)

ryantxu · 2023-03-08T20:29:46Z

pkg/tsdb/prometheus/models/query_test.go

+			"interval": "1s",
+			"intervalMs": 1000,


Are these changes still necessary?

Let me check them once more. Probably we won't need them anymore. Good catch.

Co-authored-by: Ryan McKinley <ryantxu@gmail.com>

pkg/tsdb/prometheus/models/query.go

ryantxu

@itsmylife -- this looks like a great step forward. My only remaining comments would be to improve the comments so that a noice looking at the fields+comments could understand what they do.

But that can also evolve as we have more people look at and use this :)

Co-authored-by: Ryan McKinley <ryantxu@gmail.com>

public/app/plugins/datasource/prometheus/dataquery.cue

sdboyer

agreed with @ryantxu, i think this is good enough to go in - and be marked as experimental. While it's absolutely true that the comments here are crucial for the as-code user use case, it takes a different mindset and knowledge base than just getting the types hooked up. It's OK to plan for a follow-up to improve them.

Thanks for all your work on this!

Co-authored-by: sam boyer <sdboyer@grafana.com>

gtk-grafana

LGTM, thanks for all the hard work on this, I'm excited to have type synchronization across the stacks!

bohandley

LGTM! The comments make sense to me as well, good work!

* Schematize prometheus * revert changes * close response body * Update report.json * Update pkg/tsdb/prometheus/models/query.go Co-authored-by: sam boyer <sdboyer@grafana.com> * Use without pointers * remove unused * Specify query format * Rename * Clean up schema * Update public/app/plugins/datasource/prometheus/dataquery.cue Co-authored-by: Ryan McKinley <ryantxu@gmail.com> * Update pkg/tsdb/prometheus/models/query.go Co-authored-by: Ryan McKinley <ryantxu@gmail.com> * Clean up tests * Update public/app/plugins/datasource/prometheus/dataquery.cue Co-authored-by: sam boyer <sdboyer@grafana.com> * make gen-cue * Add comments * Make linter happy * Remove editormode override * Update --------- Co-authored-by: sam boyer <sdboyer@grafana.com> Co-authored-by: Ryan McKinley <ryantxu@gmail.com>

itsmylife added 3 commits February 28, 2023 14:20

Schematize prometheus

f7e626b

revert changes

9b2d3fa

Merge branch 'main' into ismail/prometheus-schematization

83e2156

# Conflicts: # public/app/plugins/datasource/prometheus/types.ts

itsmylife added datasource/Prometheus no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Feb 28, 2023

itsmylife added this to the 9.5.0 milestone Feb 28, 2023

itsmylife requested review from sdboyer and ryantxu February 28, 2023 15:36

itsmylife requested review from a team and Eve832 as code owners February 28, 2023 15:36

itsmylife mentioned this pull request Feb 28, 2023

Prometheus: Use generated types #61872

Closed

grafanabot added area/backend area/frontend type/docs labels Feb 28, 2023

ryantxu reviewed Mar 1, 2023

View reviewed changes

ryantxu mentioned this pull request Mar 1, 2023

Prometheus: Track request.requestId rather than custom query.requestId #63913

Merged

itsmylife added 3 commits March 1, 2023 21:34

close response body

a0be26b

Merge branch 'main' into ismail/prometheus-schematization

850fdf7

Update report.json

3cdc49c

sdboyer suggested changes Mar 7, 2023

View reviewed changes

itsmylife and others added 4 commits March 7, 2023 13:05

Update pkg/tsdb/prometheus/models/query.go

fc3ba9b

Co-authored-by: sam boyer <sdboyer@grafana.com>

Use without pointers

bc84e5b

remove unused

5987de2

Specify query format

a0bd621

itsmylife requested review from sdboyer and ryantxu March 7, 2023 14:00

Clarity-89 reviewed Mar 7, 2023

View reviewed changes

public/app/plugins/datasource/prometheus/dataquery.gen.ts Outdated Show resolved Hide resolved

itsmylife added 2 commits March 7, 2023 15:24

Rename

20126de

Clean up schema

d4db739

ryantxu reviewed Mar 8, 2023

View reviewed changes

public/app/plugins/datasource/prometheus/dataquery.cue Outdated Show resolved Hide resolved

ryantxu reviewed Mar 8, 2023

View reviewed changes

Update public/app/plugins/datasource/prometheus/dataquery.cue

1abe703

Co-authored-by: Ryan McKinley <ryantxu@gmail.com>

ryantxu reviewed Mar 8, 2023

View reviewed changes

pkg/tsdb/prometheus/models/query.go Show resolved Hide resolved

ryantxu approved these changes Mar 8, 2023

View reviewed changes

Update pkg/tsdb/prometheus/models/query.go

2c536f5

Co-authored-by: Ryan McKinley <ryantxu@gmail.com>

sdboyer reviewed Mar 8, 2023

View reviewed changes

public/app/plugins/datasource/prometheus/dataquery.cue Outdated Show resolved Hide resolved

sdboyer approved these changes Mar 8, 2023

View reviewed changes

itsmylife and others added 5 commits March 8, 2023 21:48

Clean up tests

5210c5b

Update public/app/plugins/datasource/prometheus/dataquery.cue

1ce6848

Co-authored-by: sam boyer <sdboyer@grafana.com>

make gen-cue

1aa6ba9

Add comments

49afb4e

Make linter happy

ef56170

gtk-grafana approved these changes Mar 8, 2023

View reviewed changes

Remove editormode override

65855b9

bohandley approved these changes Mar 8, 2023

View reviewed changes

itsmylife added 2 commits March 9, 2023 11:04

Merge branch 'main' into ismail/prometheus-schematization

a1735ee

Update

b0f7094

itsmylife merged commit 68b588b into main Mar 9, 2023

itsmylife deleted the ismail/prometheus-schematization branch March 9, 2023 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prometheus schematization #63878

Prometheus schematization #63878

itsmylife commented Feb 28, 2023

ryantxu Mar 1, 2023

sdboyer Mar 2, 2023

itsmylife Mar 8, 2023 •

edited

Loading

ryantxu Mar 1, 2023

sdboyer Mar 7, 2023

itsmylife Mar 7, 2023

itsmylife Mar 7, 2023

sdboyer left a comment

sdboyer Mar 7, 2023

ryantxu Mar 8, 2023

ryantxu Mar 8, 2023

ryantxu Mar 8, 2023

sdboyer Mar 8, 2023

itsmylife Mar 8, 2023

sdboyer Mar 8, 2023

ryantxu Mar 8, 2023

itsmylife Mar 8, 2023

ryantxu left a comment

sdboyer left a comment

gtk-grafana left a comment

bohandley left a comment

Prometheus schematization #63878

Prometheus schematization #63878

Conversation

itsmylife commented Feb 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsmylife Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryantxu left a comment

Choose a reason for hiding this comment

sdboyer left a comment

Choose a reason for hiding this comment

gtk-grafana left a comment

Choose a reason for hiding this comment

bohandley left a comment

Choose a reason for hiding this comment

itsmylife Mar 8, 2023 •

edited

Loading