Join PipeHistory queries with pipes for additional tags

[yogevyuval]

What does this PR do?

This PR adds 2 additional tags to the PipeHistory query

Motivation

Without these tags, these metrics are almost useless. Imagine the following:
2 schemas, one is dev env, one is production. Each schema has a pipe named “INPUT_PIPE” that ingests data into snowflake.
Without these tags, the metric is useless, because it aggregates across non related schemas.

• Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
• Changelog entries must be created for modifications to shipped code
• Add the qa/skip-qa label if the PR doesn't need to be tested during QA.

[codecov]

Codecov Report

Merging #16222 (1d0ebab) into master (a6de287) will increase coverage by 0.09%.
Report is 1 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files

Flag	Coverage Δ
activemq	?
cassandra	?
hive	?
hivemq	?
ignite	?
jboss_wildfly	?
kafka	?
presto	?
snowflake	95.95% <ø> (ø)
solr	?
tomcat	?

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

[github-actions]

Test Results

  2 files    2 suites   12s ⏱️
58 tests 58 ✔️ 0 💤 0 ❌
60 runs  58 ✔️ 2 💤 0 ❌

Results for commit 1d0ebab.

♻️ This comment has been updated with latest results.

[yogevyuval]

@hestonhoffman Thanks
First contriubtion for me here - how do I fix the changelog parts? + not sure about the tests here, will they fail if the query is wrong?

[hestonhoffman]

@hestonhoffman Thanks First contriubtion for me here - how do I fix the changelog parts? + not sure about the tests here, will they fail if the query is wrong?

That's a good question for @DataDog/agent-integrations :)

[iliakur]

@yogevyuval would you mind undoing this change, updating your ddev to the latest version and running ddev release changelog new? We recently reworked the way we generate changelogs.

[yogevyuval]

@iliakur Done. Can you take a look again?
Also - anyone can help with validating and tests?

[iliakur]

Hmm, not sure why this file got added. Was this the result of running the ddev command?

Could you post the output of git status here? After we remove this file, the PR lgtm!

[yogevyuval]

Looks like a mistake, i removed it.
Any way to confirm my query does not break anything?

[yogevyuval]

@iliakur What's next needed here? Anyway to validate the query?

[iliakur]

@yogevyuval sorry about the delay. I'm about to go on holiday, my colleague @alopezz will take over next week. Have a good weekend and thanks again for your patience!

[alopezz]

Sadly, we don't have an automated way of testing the integration against an actual instance of Snowflake. If you want, you can add a test with a mocked response such as those on https://github.com/DataDog/integrations-core/blob/master/snowflake/tests/test_queries.py, but since I don't see one already in place for this query, I'm not going to block the PR based on that.

I'm going to test the PR out manually to make sure the query is valid before approval.

[yogevyuval]

Sadly, we don't have an automated way of testing the integration against an actual instance of Snowflake. If you want, you can add a test with a mocked response such as those on https://github.com/DataDog/integrations-core/blob/master/snowflake/tests/test_queries.py, but since I don't see one already in place for this query, I'm not going to block the PR based on that.

I'm going to test the PR out manually to make sure the query is valid before approval.

Fixed the PR comments (and adjusted the changelog entry accordingly).
Appreciate you helping out with manual testing - let me know if everything worked our or if additional changes are needed!

[yogevyuval]

@alopezz Did you have a chance to run this? Anything else from my side needed?

[alopezz]

Sorry for the delay. I was trying to get the necessary data into a test account to get some data from the modified query but I haven't been able to do so. The query itself is fine in that it doesn't produce an error, I'm simply not getting any results for extra confirmation.

Would you be able to share some example results of that query? Since you've found the need to improve the query I suppose you should be able to get some data from it. I can then do some final testing out of that.

[yogevyuval]

Sorry for the delay. I was trying to get the necessary data into a test account to get some data from the modified query but I haven't been able to do so. The query itself is fine in that it doesn't produce an error, I'm simply not getting any results for extra confirmation.

@alopezz
Yes, I ran this query on my account and it worked well. The reason I was a bit worried is actually things like specifying the database + schema before the table name or not, because for example in the original code it uses FROM pipe_history without any db or schema, and in the join I added I specified snowflake.account_usage.pipes.

So I wanted to make the query actually compiles well, no permission errors or other things like that

[alopezz]

@alopezz Yes, I ran this query on my account and it worked well. The reason I was a bit worried is actually things like specifying the database + schema before the table name or not, because for example in the original code it uses FROM pipe_history without any db or schema, and in the join I added I specified snowflake.account_usage.pipes.

So I wanted to make the query actually compiles well, no permission errors or other things like that

I see what you mean (I hadn't payed close enough attention to that); technically it shouldn't matter because that query is always meant to be run against the SNOWFLAKE database and is only available when the schema is set to ACCOUNT_USAGE(see config), so both qualifying and not qualifying the table name works. You might want to remove the qualification from the reference to the pipes for consistency with the rest of the existing queries, other than that I consider this ready to merge.

[yogevyuval]

@alopezz Yes, I ran this query on my account and it worked well. The reason I was a bit worried is actually things like specifying the database + schema before the table name or not, because for example in the original code it uses FROM pipe_history without any db or schema, and in the join I added I specified snowflake.account_usage.pipes.
So I wanted to make the query actually compiles well, no permission errors or other things like that

I see what you mean (I hadn't payed close enough attention to that); technically it shouldn't matter because that query is always meant to be run against the SNOWFLAKE database and is only available when the schema is set to ACCOUNT_USAGE(see config), so both qualifying and not qualifying the table name works. You might want to remove the qualification from the reference to the pipes for consistency with the rest of the existing queries, other than that I consider this ready to merge.

@alopezz I removed the qualifications

[alopezz]

Nice, thanks for your patience!

Stale