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

Integrations common lib #1028

Merged
merged 69 commits into from
Oct 18, 2023
Merged

Integrations common lib #1028

merged 69 commits into from
Oct 18, 2023

Conversation

v-zhuravlev
Copy link
Contributor

@v-zhuravlev v-zhuravlev commented Jul 24, 2023

  1. This commonlib implements
    the following panel groups:
    generic as base group, then:
  • system
  • cpu
  • memory
  • disk
  • network

Inside each group, the following panel types can be found:

  • timeSeries
  • table
  • statusHistory
  • stat
  1. Also implements common grafana annotations prototypes (such as reboot event)
  2. Adds some util functions to manipulate labels and variables

All panels have new() method to create this type of panel and stylize() method now that can be used to 'mutate' old panels or change style of existing panels.
You can also use it as stylize(allLayers=false) to ignore base styles and apply only top layer. This can be handy when you want to cherry-pick different style sets from different panels to compose new combination.

For actual use of this lib see #1050.

@@ -0,0 +1 @@
import 'github.com/grafana/grafonnet/gen/grafonnet-latest/main.libsonnet'
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest pinning to a specific Grafana version and let Grafana handle upgrading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hold to v10.0.0

local g = import 'g.libsonnet';

{
stat: import 'panels/stat/main.libsonnet',
Copy link
Contributor

Choose a reason for hiding this comment

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

does this import work? Won't it be looking in the JSONNET_PATH for this lib? I'd say ./panels/... would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, thnx

Copy link
Contributor

@rgeyer rgeyer left a comment

Choose a reason for hiding this comment

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

Several questions, mostly open-ended for the sake of discussion.

I generally like the idea that we're being opinionated, as that should be the purpose of this library. This will require fairly comprehensive documentation to explain the actual result of these assumptions/opinions. For example, in the stats info panel, I agree with the assertions, but the description of "simple info panel with text or count" falls short in fully describing what type of panel you get, and when you'd use it.

An illustrated guide with screenshots and use-cases would likely be appropriate.

common-lib/common/panels/all/base.libsonnet Outdated Show resolved Hide resolved
common-lib/common/panels/stat/base.libsonnet Outdated Show resolved Hide resolved
common-lib/common/panels/stat/percentage.libsonnet Outdated Show resolved Hide resolved
common-lib/common/panels/timeSeries/network/base.libsonnet Outdated Show resolved Hide resolved
@v-zhuravlev
Copy link
Contributor Author

Several questions, mostly open-ended for the sake of discussion.

I generally like the idea that we're being opinionated, as that should be the purpose of this library. This will require fairly comprehensive documentation to explain the actual result of these assumptions/opinions. For example, in the stats info panel, I agree with the assertions, but the description of "simple info panel with text or count" falls short in fully describing what type of panel you get, and when you'd use it.

An illustrated guide with screenshots and use-cases would likely be appropriate.

Agree, more detailed descriptions are necessary.

common-lib/common/panels.libsonnet Outdated Show resolved Hide resolved
common-lib/common/panels/all/stat/info.libsonnet Outdated Show resolved Hide resolved
@v-zhuravlev v-zhuravlev force-pushed the vzhuravlev/common-lib branch 2 times, most recently from 6c99609 to a15b493 Compare September 27, 2023 17:57
@v-zhuravlev v-zhuravlev marked this pull request as ready for review October 11, 2023 17:29
@v-zhuravlev v-zhuravlev requested a review from rgeyer October 11, 2023 17:56
Copy link
Contributor

@rgeyer rgeyer left a comment

Choose a reason for hiding this comment

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

Looking very good.. I have a few nits/questions. Enough that I'll refrain from approving just yet.

The biggest concern is that there seem to be several commits/changed files which don't relate to this PR.

Tremendous work! Thank you for the dedication to getting this finished!

All panels in this lib should implement one of the following methods:

- `panel.new(title,targets,description)` - creates new panel. List of arguments could vary;
- `panel.stylize(allLayers=true)` - directly applies this panel style to existing panel. By default includes all layers of styles. To apply only top layer, set allLayers=false. This mode is useful to cherry-pick style layers to create new style combination.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we document the concept of "layers" here a bit more thoroughly? The concept is not immediately apparent from the CONTRIB.md or README.md.

It looks like what is meant is if you, for example, apply stylize from panels/network/timeseries/packets/, with allLayers you will effectively apply not only the styles specific to a network packets timeseries (ingress above 0, egress below 0), but also the default style from panels/generic/timeseries?

Possibly an architecture diagram of the layout of this lib would be useful.

Also, I would advise that this get added to a follow-up PR, and we create an issue to track. No need to hold the merging of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like what is meant is if you, for example, apply stylize from panels/network/timeseries/packets/, with allLayers you will effectively apply not only the styles specific to a network packets timeseries (ingress above 0, egress below 0), but also the default style from panels/generic/timeseries?

correct. I'll try to expand documentation once we came to terms.

aerospike-mixin/.lint Outdated Show resolved Hide resolved
common-lib/common/panels/all/base.libsonnet Outdated Show resolved Hide resolved
title,
target,
):
super.new(title, target)
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no built-in logic for identifying fatal/critical events, which I believe is intentional. A user would define this by providing a target with a query which only surfaces fatal and critical events, yes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you got it right. We just provide this annotation.fatal as prototype, where color choice is enforced for now.

target,
):
super.new(title, target)
+ annotation.withIconColor('light-purple')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a fan of purple... But.. Wouldn't fatal or critical events be red/orange?

I haven't gotten through the other panels yet, but perhaps this is part of the embedded design language that's used throughout the library?

If so, this is another good opportunity for expanded documentation. I.E. A table of colors, and where/why they're used. Also examples on how to override those values if you wish.

Again, this is probably appropriate for a followup PR, with an issue to track.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh perfect! Still worth documenting it (again, in a future PR).

I have a bias with purple. It's my favorite color, so I don't immediately associate it with "bad things". But I'm always reminded that it's "darker" than red, and thus more severe. :)

common-lib/common/panels/disk/table/usage.libsonnet Outdated Show resolved Hide resolved
common-lib/common/panels/generic/base.libsonnet Outdated Show resolved Hide resolved
@v-zhuravlev v-zhuravlev force-pushed the vzhuravlev/common-lib branch from d31a4bc to 916695f Compare October 17, 2023 14:31
@v-zhuravlev v-zhuravlev requested a review from rgeyer October 17, 2023 14:43
Copy link
Contributor

@rgeyer rgeyer 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 regular updates and getting this sorted out. I think we're ready to merge this!

Again tremendous work! Great job!

@v-zhuravlev v-zhuravlev force-pushed the vzhuravlev/common-lib branch from 0edca29 to cfc250f Compare October 18, 2023 08:22
@v-zhuravlev v-zhuravlev enabled auto-merge (squash) October 18, 2023 08:23
@v-zhuravlev v-zhuravlev merged commit cfd1dfe into master Oct 18, 2023
@v-zhuravlev v-zhuravlev deleted the vzhuravlev/common-lib branch October 18, 2023 08:23
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.

3 participants