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

Add column measurement_db_type to output of all queries #8464

Merged
merged 2 commits into from
Dec 3, 2020

Conversation

IgorKuchmienko
Copy link
Contributor

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

This column is used to distinguish between outputs of queries that have the same value in column [measurement], such as sqlAzureDBResourceStats and sqlAzureMIResourceStats.

This column is used to distinguish between outputs of queries that have the same value in column [measurement], such as sqlAzureDBResourceStats and sqlAzureMIResourceStats.
@sjwang90 sjwang90 added area/sqlserver feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Nov 24, 2020
Copy link
Contributor

@denzilribeiro denzilribeiro left a comment

Choose a reason for hiding this comment

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

Looks fine to me. @Trovalo - any objections to this

Copy link
Contributor

@denzilribeiro denzilribeiro left a comment

Choose a reason for hiding this comment

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

Realized that DatabaseType won't always exist if someone isn't using it in the config file ( aka are using the older V2/V1 queries). May have to check if it is configured in conf file.

@Trovalo
Copy link
Collaborator

Trovalo commented Nov 24, 2020

@denzilribeiro I see no problem in adding this.
Personally, I'll leave the V2 queries as they are and add the column only on the new ones. If we want to provide that info also for old query we can easily use SERVERPORPERTY('EngineEdition') to identify the correct value

@denzilribeiro
Copy link
Contributor

Personally, I'll leave the V2 queries as they are and add the column only on the new ones. If we want to provide that info also for old query we can easily use SERVERPORPERTY('EngineEdition') to identify the correct value

But where it was being added was when results were being sent , appending a tag to all result sets which won't be conditional at this point.

@Trovalo
Copy link
Collaborator

Trovalo commented Nov 24, 2020

Sorry I didn't notice that, in any case it's fine to me.
The other option is to add the info in the query itself as a static tag (like the measurement), I have no preference, both methods are fine to me.

If we choose this way, adding a check to ensure that there is a valid value seems reasonable to me, otherwise we could end up with an empty tag, which is not that handy to query

@IgorKuchmienko
Copy link
Contributor Author

I added a check to ensure that the new column is only added when DatabaseType is not empty.

Copy link
Collaborator

@Trovalo Trovalo left a comment

Choose a reason for hiding this comment

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

Looks fine to me, @denzilribeiro anything else to add?

Copy link
Contributor

@denzilribeiro denzilribeiro left a comment

Choose a reason for hiding this comment

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

Looks good to me

@IgorKuchmienko
Copy link
Contributor Author

Looks like there is nothing else to be done here. @ssoroka can you please merge it?

@ssoroka ssoroka merged commit a267570 into influxdata:master Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sqlserver feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants