feat(plugins): Datadog for metrics collection by bisakhmondal · Pull Request #5372 · apache/apisix

bisakhmondal · 2021-10-29T15:34:08Z

What this PR does / why we need it:

This PR proves the backbone for integrating Datadog plugin with Apache APISIX.
At present it provides the following metrics:

request.count - counter
request.latency - histogram
upstream.latency - histogram
apisix.latency - histogram
ingress.size - timer
egress.size - timer

To enable datadog plugin via route, just run the following snippet

{
    "name": "my_route",
    "uri": "/hello",
    "upstream": {
        "type": "roundrobin",
        "nodes": {
            "127.0.0.1:80": 1
        }
    },
    "plugins":{
        "datadog":{},
    }
}

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

bisakhmondal · 2021-11-01T06:19:16Z

CI error not related to this PR: https://github.com/apache/apisix/runs/4064450756?check_suite_focus=true#step:14:680

…eal backend]

spacewander · 2021-11-05T08:18:49Z

+message received: apisix.upstream.latency
+message received: apisix.apisix.latency
+message received: apisix.ingress.size
+message received: apisix.egress.size


Let's add 3 more tests:

request twice & check the dump

configure namespace in the metadata and check the different namespace

configure tags in the metadata and check the difference

spacewander · 2021-11-05T08:20:09Z

+end
+
+local function generate_tag(conf, const_tags)
+    local tags = ""


Is it faster if we can use table.concat to concat the string instead?

You are right,

Lua strings are immutable

effectively it will be O(N^2) instead of O(N) without a table. updating accordingly

spacewander · 2021-11-07T12:21:33Z

+    local entry = fetch_log(ngx, {})
+    entry.upstream_latency = ctx.var.upstream_response_time * 1000
+    entry.balancer_ip = ctx.balancer_ip or ""
+    entry.route_name = ctx.route_name or ""


We should fall back to route_id if the name is missing. And need a test for it.

The code block for fallback logic is at L86
Okay, adding tests.

I think we can use route_name even there is only route_id? Therefore the name of tag is consistent between routes.

Perfectly makes sense, Updated accordingly. Thanks for pointing it out.

@spacewander ecommend using route name, won't the same routes name be confusing?

Hi! @tzssangglass thanks for the honest concern. Yeah, in a badly designed system, it might be an issue to segregate metrics of multiple routes having the same route_name. But users can always pick multiple tags for grouping (for eg. route_name:abc, service_id:12), provided they are designing APISIX infra with service level abstraction (just one example, they can use "balancer_ip, consumer" also for filtering).

@spacewander recommended using name over id because it's more readable to the user and there was this requirement in the Prometheus plugin (#5149). Do you think that we should add a param prefer_name at the plugin schema or we should defer it for the upcoming versions?
Please do let me know. Thanks.

I get. I think it is up to the community to decide if prefer_name is needed like the prometheus plugin.I suggest adding a note about that it could be misleading when route name could be duplicated for multiple.

bisakhmondal · 2021-11-08T17:47:08Z

+- **route_name**: Name specified in the route schema definition. If not present, it will fall back to the route id value.
+  - Note: If multiple routes have the same name duplicated, we suggest you to visualize graphs on the Datadog dashboard over multiple tags that could compositely pinpoint a particular route/service. If it's still insufficient for your needs, feel free to drop a feature request at [apisix/issues](https://github.com/apache/apisix/issues).


@tzssangglass I hope this would be sufficient. WDYT?

moonming · 2021-11-09T10:00:57Z

+| ----------- | ------  | ----------- |      -------       | -----         | ---------------------------------------------------------------------- |
+| host        | string  | optional    |  "127.0.0.1"       |               | The DogStatsD server host address                                      |
+| port        | integer | optional    |    8125            |               | The DogStatsD server host port                                         |
+| namespace   | string  | optional    |    "apisix"        |               | Prefix for all the custom metrics sent by APISIX agent. Useful for finding entities for metric graph. e.g. (apisix.request.counter)                                        |


@bisakhmondal please add doc and examples for how to configure host and port, thanks

Hi, @moonming do you mean how to update datadog metadata by calling the respective endpoint? Or Is it regarding the setup of a local dogstatsd agent?
Thanks

I means how to set the datadog endpoint

Yes, take this as an example: If I'm a newbie to use Apache APISIX and the datadog plugin, I'd like to know all the necessary steps after Apache APISIX runs successfully.

Like when I should update its metadata, and how I should setup its data with a Route.

Got your point. Thanks for the elaborate explanation.
The plugin has a default metadata schema that assumes dogstatsd is available at localhost:8125. If someone needs to reconfigure that I have covered that on the blog which is soon going to be exported at apisix blog
Here is the link: https://blog.bisakh.com/blog/Monitoring-with-Datadog-in-Apache-APISIX

Do you think, we should add it here (the Steps to Run Datadog Agent section & Custom Configuration)? Thanks guys

Ya, I have read your blog[1], I think the Custom Configuration part is what @moonming and I need 😄

[1] https://blog.bisakh.com/blog/Monitoring-with-Datadog-in-Apache-APISIX

Cool Cool, I'll open a PR with the code snippet and some extra details. Thanks, @juzhiyuan @moonming

bisakhmondal added 2 commits October 29, 2021 20:57

datadog plugin

66eb976

existing apis refactoring

3acdc46

bisakhmondal changed the title ~~feat: datadog plugin for metrics collection~~ feat(plugins): datadog plugin for metrics collection Oct 29, 2021

bisakhmondal changed the title ~~feat(plugins): datadog plugin for metrics collection~~ feat(plugins): Datadog for metrics collection Oct 29, 2021

tests

bbb29e3

spacewander marked this pull request as draft October 31, 2021 12:00

nginx tests

960915d

spacewander reviewed Nov 1, 2021

View reviewed changes

bisakhmondal marked this pull request as ready for review November 1, 2021 06:56

spacewander reviewed Nov 1, 2021

View reviewed changes

Comment thread t/plugin/datadog.t

bisakhmondal added 4 commits November 1, 2021 16:15

revamped implementation with batch processor: [todo: test with mock/r…

2f409ea

…eal backend]

mock testing dogstatsd

ced7a16

fix mock testing

5eaa616

fix error related to batchprocessor

129eb47

spacewander reviewed Nov 5, 2021

View reviewed changes

Comment thread t/plugin/datadog.t Outdated

bisakhmondal added 3 commits November 5, 2021 15:57

perf: string concat to table concat

c0decea

new tests

45a7f02

code suggestion

33542c6

bisakhmondal commented Nov 5, 2021

View reviewed changes

Comment thread apisix/plugins/datadog.lua Outdated

spacewander reviewed Nov 5, 2021

View reviewed changes

Comment thread apisix/plugins/datadog.lua Outdated

spacewander reviewed Nov 5, 2021

View reviewed changes

Comment thread apisix/plugins/datadog.lua Outdated

bisakhmondal added 4 commits November 5, 2021 19:03

route_name instead of id

bae5194

datadog plugin docs

9d18471

mispelt fix

f183492

suggestions

e85e9c9

spacewander reviewed Nov 7, 2021

View reviewed changes

Comment thread t/plugin/datadog.t Outdated

Comment thread t/plugin/datadog.t Outdated

Comment thread t/plugin/datadog.t Outdated

Comment thread docs/en/latest/plugins/datadog.md

Comment thread docs/en/latest/plugins/datadog.md

Comment thread apisix/plugins/datadog.lua Outdated

spacewander reviewed Nov 7, 2021

View reviewed changes

spacewander reviewed Nov 8, 2021

View reviewed changes

Comment thread docs/en/latest/plugins/datadog.md Outdated

spacewander reviewed Nov 8, 2021

View reviewed changes

Comment thread apisix/plugins/datadog.lua Outdated

tests for route_id, docs modification

2994255

spacewander approved these changes Nov 8, 2021

View reviewed changes

spacewander requested review from bzp2010, shuaijinchao and tzssangglass November 8, 2021 10:13

note for duplicate route names

3bd09cb

bisakhmondal commented Nov 8, 2021

View reviewed changes

tzssangglass approved these changes Nov 9, 2021

View reviewed changes

spacewander merged commit da691b9 into apache:master Nov 9, 2021

moonming reviewed Nov 9, 2021

View reviewed changes

juzhiyuan mentioned this pull request Nov 11, 2021

Proposal: support Plugin Metadata API apache/apisix-dashboard#2148

Closed

bisakhmondal mentioned this pull request Nov 11, 2021

docs: custom configuration for datadog plugin #5486

Merged

4 tasks

		- route_name: Name specified in the route schema definition. If not present, it will fall back to the route id value.
		- Note: If multiple routes have the same name duplicated, we suggest you to visualize graphs on the Datadog dashboard over multiple tags that could compositely pinpoint a particular route/service. If it's still insufficient for your needs, feel free to drop a feature request at [apisix/issues](https://github.com/apache/apisix/issues).

Uh oh!

Conversation

bisakhmondal commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it:

Pre-submission checklist:

Uh oh!

bisakhmondal commented Nov 1, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bisakhmondal Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

bisakhmondal commented Oct 29, 2021 •

edited

Loading

bisakhmondal Nov 5, 2021 •

edited

Loading