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 envoy dashboard #95

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add envoy dashboard #95

wants to merge 2 commits into from

Conversation

heydbut
Copy link
Contributor

@heydbut heydbut commented May 25, 2023

Description

What does this PR do?
Add lastest envoy dashboard

PR checklist

Please confirm the following items:

  • At least one screenshot of the proposed dashboard is included in this PR. If you're proposing substantive changes to queries or new queries then please ensure your screenshot shows relevant data.
  • All chart names are in Title case.
  • All query names are lower case letters, beginning the first query of each chart as "a" and proceeding alphabetically ... "b", "c", etcetera.

app lightstep com_dev_bcronin_dashboard_envoy-overview_BCvSNpvX (1)

@heydbut heydbut requested a review from nslaughter May 25, 2023 19:36
@heydbut heydbut self-assigned this May 25, 2023
Copy link
Contributor

@altuner altuner left a comment

Choose a reason for hiding this comment

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

LGTM

}
group {
rank = 0
title = ""
Copy link
Collaborator

@nslaughter nslaughter Sep 13, 2023

Choose a reason for hiding this comment

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

The group without a title seems wrong. Oversight? Trying to accomplish some formatting with the empty group?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should not remove this. It's the default unamed group for some charts

query_name = "((1-(a/b))*100)"
display = "big_number"
hidden = false
query_string = "with\n a = metric envoy_cluster_upstream_rq_xx | filter (((envoy_response_code_class == \"5\") || (envoy_response_code_class == 5)) || (envoy_response_code_class == 5.0)) | rate 5m, 5m | group_by [], mean;\n b = metric envoy_cluster_upstream_rq_completed | rate 5m, 5m | group_by [], mean;\njoin (((1-(a / b))*100)), a=0, b=0 | reduce 5m, mean"
Copy link
Collaborator

@nslaughter nslaughter Sep 13, 2023

Choose a reason for hiding this comment

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

I find the name confusing. Looks like the metric is 1 - (5XXs / Completed). In this context I find it confusing to call that a Success Rate, because I think of that as 2XXs. Not sure but also seems like we'd group_by the code class, but hard to tell the intent without the context of the whole dashboard.

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's the new version of envoy. I find it confusing but I translated an existing dashboard and it was like this

Copy link
Collaborator

@nslaughter nslaughter left a comment

Choose a reason for hiding this comment

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

I put the comments in about questions I had. It seems we need to remove an errant empty group, but otherwise it's really just a couple of things I found surprising and would like to understand. Let's sync on this ASAP and close.

}

resource "lightstep_dashboard" "otel_collector_envoy_dashboard" {
project_name = var.cloud_observability_project
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
project_name = var.cloud_observability_project
project_name = var.lightstep_project

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