Skip to content

Consul Kubernetes Datadog Integration Docs Update#20661

Merged
natemollica-nm merged 19 commits intomainfrom
natemollica-nm/datadog-kubernetes-doc-update
Feb 20, 2024
Merged

Consul Kubernetes Datadog Integration Docs Update#20661
natemollica-nm merged 19 commits intomainfrom
natemollica-nm/datadog-kubernetes-doc-update

Conversation

@natemollica-nm
Copy link
Copy Markdown
Contributor

@natemollica-nm natemollica-nm commented Feb 15, 2024

Description

Adding in an Observability piece to supplement hashicorp/consul-k8s#3407 which added
helm configuration options for configuring Datadog metrics integration with consul-k8s.

Testing & Reproduction steps

Worked with the information here to update Consul documentation website with details outlining the changes made in the PR.

Links

PR Checklist

  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@natemollica-nm natemollica-nm added type/docs Documentation needs to be created/updated/clarified backport/1.16 This release series is no longer active on CE. Use backport/ent/1.16. backport/1.17 This release series is no longer active on CE. Use backport/ent/1.17. pr/no-changelog PR does not need a corresponding .changelog entry labels Feb 15, 2024
@natemollica-nm natemollica-nm force-pushed the natemollica-nm/datadog-kubernetes-doc-update branch from 7917acb to 9adf3fc Compare February 16, 2024 00:26
@natemollica-nm natemollica-nm marked this pull request as ready for review February 16, 2024 00:32
@natemollica-nm natemollica-nm requested a review from a team as a code owner February 16, 2024 00:32
Copy link
Copy Markdown
Contributor

@boruszak boruszak left a comment

Choose a reason for hiding this comment

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

Thanks for this docs contribution!

There are some style issues that need to be sorted before I can finish reviewing the page and approve it.

Refer to the following style guides to help you format:

https://github.com/hashicorp/tutorials/blob/main/STYLE_GUIDE.md

https://github.com/hashicorp/engineering-docs/blob/main/writing/markdown.md

Comment on lines 135 to 168
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you reformat this table using Markdown instead of HTML? Link to style guide: Markdown vs. HTML tables

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

Comment on lines 19 to 23
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  1. Highlight is not appropriate here. Refer to the Alerts section in the style guide for more info.
  2. Even if you use an alert, this one is too long. So format it without the alert and we can adjust from there.
  3. Use Markdown instead of HTML.

Copy link
Copy Markdown
Contributor Author

@natemollica-nm natemollica-nm Feb 16, 2024

Choose a reason for hiding this comment

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

Done!

Comment on lines 289 to 345
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add a sentence between these headings to describe the section? And a sentence after "Helm CHart configuration?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Use of this method will map to Datadog as described in [Mapping Prometheus Metrics to Datadog Metrics](https://docs.datadoghq.com/integrations/guide/prometheus-metrics/?tab=latestversion) and summarized below.
Use of this method maps to Datadog as described in [Mapping Prometheus Metrics to Datadog Metrics](https://docs.datadoghq.com/integrations/guide/prometheus-metrics/?tab=latestversion). The following table summarizing how these metrics map to each other.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated!

Comment on lines 314 to 414
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
| OpenMetrics Metric Type | Datadog Metric Type |
|-------------------------|------------------------------------|
| `Gauge` | `gauge` |
| `Counter` | `count` |
| Histogram: `_count ` | `count.count` |
| Histogram: `_sum` | `count.sum` |
| Histogram: `_bucket` | `count.bucket` \|\| `distribution` |
| Summary: `_count` | `count.count` |
| Summary: `_sum` | `count.sum` |
| Summary: `sample` | `gauge.quantile` |
| OpenMetrics metric type | Datadog metric type |
| :------------------------ | :----------------------------------- |
| `Gauge` | `gauge` |
| `Counter` | `count` |
| Histogram: `_count ` | `count.count` |
| Histogram: `_sum` | `count.sum` |
| Histogram: `_bucket` | `count.bucket` \|\| `distribution` |
| Summary: `_count` | `count.count` |
| Summary: `_sum` | `count.sum` |
| Summary: `sample` | `gauge.quantile` |

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated!

Comment on lines 326 to 329
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure what your intent is with this section? The list isn't described so I can't tell what info you want to communicate to the user.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right. Don't know why this was as a list. Thank you! Updated!

@david-yu
Copy link
Copy Markdown
Contributor

@natemollica-nm Thank you for doing this and I think a huge boost to what we can do with DataDog. I have a high level question around placement of this doc. It's currently place under Service Mesh -> Observabililty. Are these metrics more for gathering Consul server metrics via DataDog or are these for collecting metrics for all service mesh components?

@natemollica-nm
Copy link
Copy Markdown
Contributor Author

@david-yu -- Good point to bring up on the placement. This is more Consul specific metrics collection from the servers and nothing regarding Envoy metrics. That's a separate set of annotations that should be placed on Kube pods that have dataplane containers with them. Any recommendations on better placement or where you'd like to see it?

@david-yu
Copy link
Copy Markdown
Contributor

I think moving it under Deployment Configs makes the most sense for me perhaps: https://developer.hashicorp.com/consul/docs/k8s/deployment-configurations/datadog-metrics

I would put it under Vault Secrets Backend in the nav bar.

image

@natemollica-nm
Copy link
Copy Markdown
Contributor Author

@david-yu -- Done! Just moved it over.

Datadog to Datadog metrics

Co-authored-by: David Yu <dyu@hashicorp.com>
Copy link
Copy Markdown
Contributor

@david-yu david-yu left a comment

Choose a reason for hiding this comment

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

Awesome, great work!

… set and Note on assumption for Consul RPC TLS certificate mounts
@natemollica-nm
Copy link
Copy Markdown
Contributor Author

@david-yu @boruszak -- Sorry for this last commit and push 😬. I added some additional amplifying information in regard to the Consul Datadog Checks and OpenMetrics sections (Notes) in regard to how Consul RPC TLS certificates are handles by the automated integration. Let me know if these changes are OK with you guys and I'll go ahead and merge.

Copy link
Copy Markdown
Contributor

@david-yu david-yu left a comment

Choose a reason for hiding this comment

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

Changes LGTM

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

Labels

backport/1.16 This release series is no longer active on CE. Use backport/ent/1.16. backport/1.17 This release series is no longer active on CE. Use backport/ent/1.17. pr/no-changelog PR does not need a corresponding .changelog entry pr/no-metrics-test type/docs Documentation needs to be created/updated/clarified

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants