[ECOINT-139] Stonebranch Integration for Datadog - #2866
Conversation
|
Created DOCS-12926 for Docs Team review. |
|
@Len0608 thanks for opening a new PR. You can ignore the "validate repository" check for now, but please resolve the test failures. |
|
This pull request has not been updated for more than 21 days. If there are no updates to this PR within 7 days, it will be closed. If you'd like to re-open this PR after it's been closed, you can start from the latest master branch or pull the latest changes into your branch and create a new pull request. |
|
I updated ddev to version 14.10 for linux and ran ddev lint once. It resolved with
stonebranch lintcmd [1] | ruff format --diff --check --config ../pyproject.toml . The GH actions still give the linting error for the test actions |
|
@Len0608 have you tried running |
Running the command results in the following: Running it to test all, has also shown no changes |
nubtron
left a comment
There was a problem hiding this comment.
Hi @Len0608, it might be that ddev is not configured correctly. Could you double-check the configuration, run ddev config set repo extras and then re-run ddev test --fmt stonebranch?
Also could you please commit your metadata.csv to the stonebranch directory? If you'd don't have it ready and you would like assistance on how to create it, just let me know!
|
Hello @nubtron! I have confirmed with your commands given, this is the following output: It seems that it did not change anything on the stonebranch folder itself. When checking the error of the GH action it looks like it is removing empty lines which do not even exist in the files I have uploaded. On the topic of the metadata.csv, as far as I understand this will be the metric list of all the metrics which will be scraped by the Agent correct? Edit: for completeness purposes, here is my ddev config: |
|
@Len0608 hmm I can see the empty line in the file, is it possible the fix was applied locally but didn’t make it into the push to GitHub? |
That's right! It documents the metrics that will be scraped, and lets you set some metadata such as units of measure and decription. You can also then use the "assert_metrics_using_metadata" test. It doesn't control which metrics are sent however, it only documents them - the implementation will still have to be done in Python. |
|
@nubtron after reediting the files then running ddev test --fmt stonebranch it finally worked and changed the files again. I am not sure what the issue with that is but the test checks have passed now. I have also added the metadata.csv in the extension. |
|
Hi @Len0608, thanks, will look into it shortly! In the meantime I noticed that another validation has failed, you can fix it by running |
| @@ -0,0 +1,65 @@ | |||
| metric_name,metric_type,interval,unit_name,per_unit_name,description,orientation,integration,short_name,curated_metric | |||
| stonebranch.jvm_classes_currently_loaded,gauge,60,class,,The number of classes that are currently loaded in the JVM,0,stonebranch,jvm classes loaded, | |||
There was a problem hiding this comment.
You can leave the interval empty, it will be managed by the Agent. You can change the value in stonebranch/datadog_checks/stonebranch/config_models/defaults.py if you think it's necessary but we very rarely change this value.
There was a problem hiding this comment.
I addressed the changes in the metadata. I am running into the issue of the ddev validate metadata failing. I get the following error: └── stonebranch
└── stonebranch/metadata.csv
stonebranch: metric_prefix does not exist in manifest or overrides.
From the instructions of creating the PR structure it was mentioned that there should be no manifest created. I tested it with a simple one but that just leads to more issues down the line. I removed it for now but the validate will fail due the issue mentioned above.
There was a problem hiding this comment.
Confirmed - there should be no manifest for this integration.
The override mentioned in the error message mentioned goes in .ddev/config.toml:
[overrides.metrics-prefix]
stonebranch = "stonebranch."Since the no-manifest flow is relatively new, [overrides.metrics-prefix] section is not present in that file yet and you'll need to add it.
After that the ddev metadata validation should pass. Let me know if you have other issues!
| @@ -0,0 +1,62 @@ | |||
| init_config: | |||
There was a problem hiding this comment.
The conf.yaml.example file is typically generated from spec.yaml rather than hand-crafted. Here's an example:
https://github.com/DataDog/integrations-core/blob/master/krakend/assets/configuration/spec.yaml
| @@ -0,0 +1,30 @@ | |||
| # Stonebranch | |||
There was a problem hiding this comment.
Could you follow a template similar to this, for the README?
| from datadog_checks.base import OpenMetricsBaseCheckV2 | ||
|
|
||
|
|
||
| class StonebranchCheck(OpenMetricsBaseCheckV2): |
There was a problem hiding this comment.
Could you follow the pattern here rather than using the config file to select the metrics?
The yaml config file is meant for user overrides, while check.py/metrics.py are meant for the integration developer to set.
|
Hello @nubtron, I have moved the config to the spec.yaml and updated the Readme. Does this look better now? |
|
Thanks @Len0608 for the changes! Will review shortly! |
…ucture to be able to toggle metric groups, added metric.py for listing what metrics will be scraped. Adjusted spec.yaml to create correct conf.yaml.example instead of manually changing conf.yaml.example, Updated README to be in line with other integrations.
…g description in readme. update test units to include metadata csv
Co-authored-by: Esther Kim <esther.kim@datadoghq.com>
Co-authored-by: Esther Kim <esther.kim@datadoghq.com>
Co-authored-by: Esther Kim <esther.kim@datadoghq.com>
Co-authored-by: Esther Kim <esther.kim@datadoghq.com>
Co-authored-by: Esther Kim <esther.kim@datadoghq.com>
Review from estherk15 is dismissed. Related teams and files:
- documentation
- stonebranch/README.md
- stonebranch/assets/dashboards/stonebranch_overview.json
- stonebranch/assets/monitors/agent_offline.json
- stonebranch/assets/monitors/db_pool_exhaustion.json
- stonebranch/assets/monitors/license_cluster_nodes.json
- stonebranch/assets/monitors/license_distributed_agents.json
- stonebranch/assets/monitors/license_monthly_executions.json
- stonebranch/assets/monitors/license_task_definitions.json
- stonebranch/assets/monitors/license_zos_agents.json
- stonebranch/assets/monitors/oms_disconnected.json
- stonebranch/assets/monitors/uc_node_down.json
- stonebranch/metadata.csv
nubtron
left a comment
There was a problem hiding this comment.
Hi @Len0608,
Thanks for the update!
As a first step, in metrics.py, we need to set the families of the metrics, without total.
uc_task_instance_launch_total → uc_task_instance_launch
uc_task_instance_late_start_total → uc_task_instance_late_start
uc_task_instance_late_finish_total → uc_task_instance_late_finish
uc_task_instance_early_finish_total → uc_task_instance_early_finish
uc_universal_event_total → uc_universal_event
jvm_classes_loaded_total → jvm_classes_loaded
jvm_classes_unloaded_total → jvm_classes_unloaded
jvm_memory_pool_allocated_bytes_total → jvm_memory_pool_allocated_bytes
jvm_threads_started_total → jvm_threads_started
process_cpu_seconds_total → process_cpu_seconds
After correcting the counter mapping keys, the OpenMetrics V2 class appends .count to each configured
Datadog metric name. Could you also update the corresponding metric_name values in metadata.csv to:
stonebranch.uc_task_instance.launch.total.count
stonebranch.uc_task_instance.late_start.total.count
stonebranch.uc_task_instance.late_finish.total.count
stonebranch.uc_task_instance.early_finish.total.count
stonebranch.uc_universal_event.total.count
stonebranch.jvm_classes_loaded_total.count
stonebranch.jvm_classes_unloaded_total.count
stonebranch.jvm_memory_pool_allocated_bytes_total.count
stonebranch.jvm_threads_started_total.count
stonebranch.process_cpu_seconds_total.count
uc_history already has the correct mapping key, but its metadata name also needs the suffix:
stonebranch.uc_history.total.count
Could you also update the dashboards with these new metrics?
What does this PR do?
A brief description of the change being made with this pull request.
The integration has been created by following the documentation on how to build an agent based integration from Datadog. Then implementing the check and test for the Prometheus scrape.
Motivation
What inspired you to submit this pull request?
To kick off the integration between Datadog and Stonebranch
Review checklist
no-changeloglabel attachedAdditional Notes
Anything else we should know when reviewing?