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

Establish a docs framework - Resolves #60 #84

Merged
merged 14 commits into from
Sep 16, 2022
Merged

Establish a docs framework - Resolves #60 #84

merged 14 commits into from
Sep 16, 2022

Conversation

rgeyer
Copy link
Collaborator

@rgeyer rgeyer commented Jun 30, 2022

Should have committed way earlier...
Added a makefile with embedmd which pulls in the usage info into the main docs file.
Created a docs directory and rules sub directory to establish heirarchy.
Created a script to replace rule names surrounded in backticks with links to their docs (if present).

Added a makefile with embedmd which pulls in the usage info into the main docs file.
Created a docs directory and rules sub directory to establish heirarchy.
Created a script to replace rule names surrounded in backticks with links to their docs (if present).
docs/index.md Outdated
Comment on lines 79 to 90
## Job and Instance Template Variables

The following rules work together to ensure that every dashboard has template variables for Job and Instance, and that they are properly configured, and used in every promql query.

* `template-job-rule`
* `template-instance-rule`
* `target-job-rule`
* `target-instance-rule`

The reasoning is that.. WIP, but...
* All prom metrics will have these, as it's part of the spec
* Other reasons?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@v-zhuravlev can you help provide some context here? I know you've raised concerns about enforcing these rules, so if we can have that discussion in the comments here, or flesh-out this document it would be appreciated.

Choose a reason for hiding this comment

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

As it turns out, there is a problem with $job (and $instance as well) filter becomes visible when you start to compose dashboards that could have panels from multiple sources. For example, prometheus node_exporter and promtail logs from the same host. Or from node_exporter and process_exporter exporters that reside on the same host.

I think that at first the idea of this rule was to have a two-level grouping of instances:

  • 1st level: instance
  • 2nd level: job

This is convenient, but since job is built-in label, using it for grouping/filtering makes prometheus/grafana-agent configs complex (if you have multiple metrics/logs sources):

  • It requires rewriting already present labels, so the match for metrics&logs , making scrape_configs complex due to additional relabelling rules.
  • Since the label is present, relabelling seems to be the only way to overwrite it. If this label were vacant then we could just used external_labels or labels to set the common job in grafana-agent/prometheus.

Here are some examples of configs where such user confusing relabeling taking place:
https://grafana.com/docs/grafana-cloud/integrations/integrations/integration-docker/#post-install-configuration-for-the-docker-integration
https://grafana.com/docs/grafana-cloud/integrations/integrations/integration-mysql/

So, we should ask, what is the main reason we enforce this?
If we need a second level of grouping, perhaps we should use label like cluster instead? (and make it optional, where it is applicable)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I went ahead and documented this in it's "current state" to get this document framework committed.

Let's revisit the effectiveness of these rules, and we can address in a follow-up PR. I'll create an issue to track.

@rgeyer rgeyer marked this pull request as ready for review July 29, 2022 19:21
Copy link
Contributor

@mshahzeb mshahzeb left a comment

Choose a reason for hiding this comment

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

LGTM! Couple of nits and comments.

.gitignore Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
scripts/replace-rulenames-with-doclinks.sh Outdated Show resolved Hide resolved
docs/index.md Show resolved Hide resolved
docs/rules/panel-units-rule.md Show resolved Hide resolved
Copy link
Contributor

@alyssawada alyssawada left a comment

Choose a reason for hiding this comment

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

Nice work, @rgeyer and thanks for your patience! Feel free to use whatever feels right and let me know if you have any questions on my suggests/comments :)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/rules/panel-datasource-rule.md Show resolved Hide resolved
rgeyer and others added 8 commits September 16, 2022 10:57
Co-authored-by: Alyssa Wada <101596687+alyssawada@users.noreply.github.com>
Co-authored-by: Alyssa Wada <101596687+alyssawada@users.noreply.github.com>
Co-authored-by: Alyssa Wada <101596687+alyssawada@users.noreply.github.com>
Co-authored-by: Alyssa Wada <101596687+alyssawada@users.noreply.github.com>
Co-authored-by: Alyssa Wada <101596687+alyssawada@users.noreply.github.com>
Co-authored-by: Alyssa Wada <101596687+alyssawada@users.noreply.github.com>
Co-authored-by: Alyssa Wada <101596687+alyssawada@users.noreply.github.com>
Co-authored-by: Alyssa Wada <101596687+alyssawada@users.noreply.github.com>
@rgeyer rgeyer merged commit fab2526 into main Sep 16, 2022
@rgeyer rgeyer deleted the feat/60 branch September 16, 2022 18:01
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.

None yet

4 participants