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

Remove id from dashboard mixin #9885

Closed
wants to merge 1 commit into from
Closed

Conversation

serathius
Copy link
Member

Grafana provisioned dashboards cannot contain id.
It causes dashboard to be skipped and error in logs

lvl=eror msg="provisioned dashboard json files cannot contain id" logger=provisioning.dashboard type=file name=default

cc @tomwilkie @brancz

@brancz
Copy link
Contributor

brancz commented Jun 26, 2018

Thanks, yes we should remove this. Could you just add a uid to ensure, that the URL is stable? In the kubernetes-mixin we just hashed the name of the dashboard file, so std.md5('etcd.json').

@tomwilkie
Copy link
Contributor

tomwilkie commented Jun 26, 2018 via email

@serathius
Copy link
Member Author

@brancz done

@brancz
Copy link
Contributor

brancz commented Jun 26, 2018

Sorry for forgetting on my last review, could you just re-generate the rendered json manifest as described in this pull request: #9789

Then this lgtm 👍

@serathius
Copy link
Member Author

done

@@ -341,7 +340,7 @@
"steppedLine": false,
"targets": [
{
"expr": "etcd_mvcc_db_total_size_in_bytes{job=\"$cluster\"}",
"expr": "etcd_debugging_mvcc_db_total_size_in_bytes{job=\"$cluster\"}",
Copy link
Contributor

Choose a reason for hiding this comment

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

@gyuho has the "debugging" namespace recently been removed? If so @serathius we should make sure this is the same in the mixin, or maybe even configurable, so the mixin is still useful for older versions of etcd.

Copy link
Contributor

Choose a reason for hiding this comment

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

@brancz Yes https://github.com/coreos/etcd/pull/9819/files. We should use etcd_mvcc_db_total_size_in_bytes instead. etcd_debugging_mvcc_db_total_size_in_bytes will still be served in 3.4 for backward compatibilities but will be dropped in 3.5.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying @gyuho in that case we should make the metric name configurable in the mixin @serathius. Could you add a etcdDbTotalSizeMetric configuration under the top level _config to configure this metric name and have it default to etcd_mvcc_db_total_size_in_bytes. Then downstream users can configure it to the metric name they need. Once 3.5 is out we can then drop the config entirely. Sorry for all the trouble @serathius , I promise once we have that down this lgtm 🙂 .

Copy link
Contributor

Choose a reason for hiding this comment

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

Kind ping @serathius could you please update per @brancz review?

@stale
Copy link

stale bot commented Apr 6, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 6, 2020
@serathius
Copy link
Member Author

No longer working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants