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

doc: http metrics path normalization #4155

Merged

Conversation

nelson-parente
Copy link
Contributor

@nelson-parente nelson-parente commented May 21, 2024

Thank you for helping make the Dapr documentation better!

Please follow this checklist before submitting:

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Read the contribution guide
  • Commands include options for Linux, MacOS, and Windows within codetabs
  • New file and folder names are globally unique
  • Page references use shortcodes instead of markdown or URL links
  • Images use HTML style and have alternative text
  • Places where multiple code/command options are given have codetabs

In addition, please fill out the following to help reviewers understand this pull request:

Description

Related code link:

dapr/proposals#58

Copy link

Stale PR, paging all reviewers

@github-actions github-actions bot added the stale label May 28, 2024
@hhunter-ms hhunter-ms removed the stale label May 28, 2024
Copy link

github-actions bot commented Jun 3, 2024

Stale PR, paging all reviewers

@github-actions github-actions bot added the stale label Jun 3, 2024
@nelson-parente nelson-parente force-pushed the feat/http-metrics-path-normalization branch from f51bd67 to 347c25c Compare June 5, 2024 09:55
@msfussell msfussell added waiting-on-code-pr The code PR needs to be merged before the docs are updated and removed stale labels Jun 8, 2024
Signed-off-by: nelson.parente <[email protected]>
@nelson-parente nelson-parente force-pushed the feat/http-metrics-path-normalization branch from eecfc0a to 5fa8028 Compare June 14, 2024 18:37
@nelson-parente nelson-parente marked this pull request as ready for review June 17, 2024 09:20
@nelson-parente nelson-parente requested review from a team as code owners June 17, 2024 09:20
@msfussell msfussell removed the waiting-on-code-pr The code PR needs to be merged before the docs are updated label Jun 19, 2024
Copy link

Stale PR, paging all reviewers

@github-actions github-actions bot added the stale label Jun 25, 2024
Copy link
Member

@msfussell msfussell left a comment

Choose a reason for hiding this comment

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

Main feedback here is to provide a little more explanation on the examples. Also the default of true remains the same in v1.14 and so need to update this everywhere

@JoshVanL
Copy link
Contributor

Please can we include in the documentation that there are security concerns with using the default high cardinality option whereby exposed Dapr enabled applications that call HTTP endpoints based on client input can cause unbounded cardinality in metrics resulting in DDoSing of the Dapr instance or cluster and/or larger metric ingestion bills.

@hhunter-ms hhunter-ms removed the stale label Jun 25, 2024
Signed-off-by: nelson.parente <[email protected]>
Signed-off-by: nelson.parente <[email protected]>
@nelson-parente nelson-parente force-pushed the feat/http-metrics-path-normalization branch from 29bda3d to eea7edc Compare June 25, 2024 17:16
Signed-off-by: nelson.parente <[email protected]>
Copy link
Collaborator

@hhunter-ms hhunter-ms left a comment

Choose a reason for hiding this comment

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

quick edits

```

In the examples above this path filter `/orders/{orderID}/items/{itemID}` would return a single metric count matching all the orderIDs and all the itemIDs rather than multiple metrics for each itemID. For more information see [HTTP metrics path matching]({{< ref "metrics-overview.md#http-metrics-path-matching" >}})
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
In the examples above this path filter `/orders/{orderID}/items/{itemID}` would return a single metric count matching all the orderIDs and all the itemIDs rather than multiple metrics for each itemID. For more information see [HTTP metrics path matching]({{< ref "metrics-overview.md#http-metrics-path-matching" >}})
In the examples above, the path filter `/orders/{orderID}/items/{itemID}` would return a single metric count matching all the `orderIDs` and all the `itemIDs`, rather than multiple metrics for each `itemID`. For more information, see [HTTP metrics path matching]({{< ref "metrics-overview.md#http-metrics-path-matching" >}}).

Signed-off-by: nelson.parente <[email protected]>
@nelson-parente
Copy link
Contributor Author

applied @hhunter-ms suggestions, ty! 👍

Copy link
Member

@msfussell msfussell left a comment

Choose a reason for hiding this comment

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

These are now an amazing detailed set of docs! Thank you @nelson-parente !

@msfussell msfussell merged commit 57b85bf into dapr:v1.13 Jun 27, 2024
7 checks passed
@hhunter-ms hhunter-ms added this to the 1.14 milestone Jun 27, 2024
salaboy pushed a commit to salaboy/docs that referenced this pull request Jun 27, 2024
* doc: http metrics path normalization

Signed-off-by: nelson.parente <[email protected]>

* doc: code review & path matching rename

Signed-off-by: nelson.parente <[email protected]>

* doc: add configuration examples

Signed-off-by: nelson.parente <[email protected]>

* update: update docs based on last proposal changes

Signed-off-by: nelson.parente <[email protected]>

* feat: more updates based on the ingress/egress merge

Signed-off-by: nelson.parente <[email protected]>

* doc: code review comments

Signed-off-by: nelson.parente <[email protected]>

* doc: code review comments

Signed-off-by: nelson.parente <[email protected]>

* feat: add excludeVerbs

Signed-off-by: nelson.parente <[email protected]>

* feat: new line

Signed-off-by: nelson.parente <[email protected]>

* feat: add review meeting changes

Signed-off-by: nelson.parente <[email protected]>

* feat: apply hunter suggestions

Signed-off-by: nelson.parente <[email protected]>

---------

Signed-off-by: nelson.parente <[email protected]>
Signed-off-by: salaboy <[email protected]>
salaboy pushed a commit to salaboy/docs that referenced this pull request Jun 27, 2024
* doc: http metrics path normalization

Signed-off-by: nelson.parente <[email protected]>

* doc: code review & path matching rename

Signed-off-by: nelson.parente <[email protected]>

* doc: add configuration examples

Signed-off-by: nelson.parente <[email protected]>

* update: update docs based on last proposal changes

Signed-off-by: nelson.parente <[email protected]>

* feat: more updates based on the ingress/egress merge

Signed-off-by: nelson.parente <[email protected]>

* doc: code review comments

Signed-off-by: nelson.parente <[email protected]>

* doc: code review comments

Signed-off-by: nelson.parente <[email protected]>

* feat: add excludeVerbs

Signed-off-by: nelson.parente <[email protected]>

* feat: new line

Signed-off-by: nelson.parente <[email protected]>

* feat: add review meeting changes

Signed-off-by: nelson.parente <[email protected]>

* feat: apply hunter suggestions

Signed-off-by: nelson.parente <[email protected]>

---------

Signed-off-by: nelson.parente <[email protected]>
Signed-off-by: salaboy <[email protected]>
@marcduiker
Copy link
Contributor

@holopin-bot @nelson-parente Thank you Nelson!

Copy link

holopin-bot bot commented Aug 15, 2024

Congratulations @nelson-parente, the maintainer of this repository has issued you a badge! Here it is: https://holopin.io/claim/clzv8p0pm145910clfsdiz7tmo

This badge can only be claimed by you, so make sure that your GitHub account is linked to your Holopin account. You can manage those preferences here: https://holopin.io/account.
Or if you're new to Holopin, you can simply sign up with GitHub, which will do the trick!

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.

6 participants