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

Fix resources not sent on service check metrics #14422

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

alexandre-normand
Copy link
Contributor

What does this PR do?

While taking over PR 14333, I made the wrong call when addressing the end to end test failure related to the internal resource tags. Instead of removing the resources from service checks that we emit, we actually want to keep the resource tags so that the service check metrics like check_run.postgres.can_connect.ok have resources set on them.

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • If the PR doesn't need to be tested during QA, please add a qa/skip-qa label.

@ghost ghost added the integration/mysql label Apr 19, 2023
@alexandre-normand alexandre-normand added this to the 7.45 milestone Apr 19, 2023
@alexandre-normand alexandre-normand changed the title DBM-2344 Fix resources not sent on service check metrics Fix resources not sent on service check metrics Apr 19, 2023
@github-actions
Copy link

Test Results

  10 files    10 suites   11m 25s ⏱️
132 tests 132 ✔️ 0 💤 0
660 runs  654 ✔️ 6 💤 0

Results for commit 1455b75.

@alexandre-normand alexandre-normand marked this pull request as ready for review April 19, 2023 22:06
@alexandre-normand alexandre-normand requested review from a team as code owners April 19, 2023 22:06
@codecov
Copy link

codecov bot commented Apr 19, 2023

Codecov Report

Merging #14422 (1455b75) into master (85ab590) will increase coverage by 0.00%.
The diff coverage is 90.90%.

Flag Coverage Δ
mysql 87.53% <90.90%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@alexandre-normand alexandre-normand added changelog/no-changelog category/bugfix For use during Agent Release period and removed changelog/Fixed labels Apr 19, 2023
@yzhan289 yzhan289 merged commit 0f3019b into master Apr 20, 2023
@yzhan289 yzhan289 deleted the alex.normand/fix-service-check-tagging-mysql branch April 20, 2023 12:40
_assert_complex_config(
aggregator,
[tags.DATABASE_INSTANCE_RESOURCE_TAG.format(hostname='stubbed.hostname')],
tags.SC_TAGS + [tags.DATABASE_INSTANCE_RESOURCE_TAG.format(hostname='stubbed.hostname')],
tags.METRIC_TAGS_WITH_RESOURCE,
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like this is asserting that the dd.resource.database_instance:* would be in this array twice? I am assuming the test case normalizes the tag set somewhere and not that the tags would show up twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah, the array only includes the internal resource tag once. tags.SC_TAGS omits the internal tag because it had to be a formatted string depending on the hostname.

Copy link
Contributor

@jmeunier28 jmeunier28 Apr 20, 2023

Choose a reason for hiding this comment

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

tags.METRIC_TAGS_WITH_RESOURCE includes it, though. Am i just reading this diff incorrectly?

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 does but one of those parameters is the service check tags and the other is the metric tags. They had to be separated because service check tags will include the internal resource tag even in the end to end case while metrics won't have the internal tags (because it gets transformed into actual resources on metrics).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category/bugfix For use during Agent Release period integration/mysql
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants