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

feat: Added IsMeasurement option to Column attribute #240

Merged
merged 3 commits into from
Oct 5, 2021

Conversation

vesuvian
Copy link
Contributor

@vesuvian vesuvian commented Sep 23, 2021

Proposed Changes

Added IsMeasurement option to Column attribute, allowing for dynamic measurement names in POCO classes

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • dotnet test completes successfully
  • Commit messages are in semantic format
  • Sign CLA (if not already signed)

@vesuvian vesuvian changed the title Added IsMeasurement option to Column attribute feat: Added IsMeasurement option to Column attribute Sep 23, 2021
@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2021

Codecov Report

Merging #240 (bc52d39) into master (533c46f) will increase coverage by 0.21%.
The diff coverage is 96.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #240      +/-   ##
==========================================
+ Coverage   85.28%   85.49%   +0.21%     
==========================================
  Files          71       72       +1     
  Lines        6366     6424      +58     
==========================================
+ Hits         5429     5492      +63     
+ Misses        937      932       -5     
Impacted Files Coverage Δ
Client.Core/Flux/Internal/AttributesCache.cs 100.00% <ø> (ø)
Client.Linq/Internal/QueryVisitor.cs 98.80% <ø> (ø)
Client.Linq/Internal/QueryAggregator.cs 98.16% <92.59%> (-0.81%) ⬇️
Client.Core/Attributes.cs 100.00% <100.00%> (ø)
Client.Core/Flux/Internal/FluxResultMapper.cs 67.92% <100.00%> (+5.17%) ⬆️
Client.Linq/IMemberNameResolver.cs 88.00% <100.00%> (+1.63%) ⬆️
Client.Linq/InfluxDBQueryable.cs 92.10% <100.00%> (+0.67%) ⬆️
Client.Linq/Internal/Expressions/ColumnName.cs 100.00% <100.00%> (ø)
...Linq/Internal/Expressions/MeasurementColumnName.cs 100.00% <100.00%> (ø)
Client.Linq/Internal/QueryExpressionTreeVisitor.cs 93.75% <100.00%> (+0.05%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 533c46f...bc52d39. Read the comment docs.

@vesuvian
Copy link
Contributor Author

I've been able to run the most immediately relevant unit tests without issue, but I'm struggling to run tests that depend on a live instance of InfluxDB. How can I configure the tests to run against my InfluxDB server?

@bednar
Copy link
Contributor

bednar commented Sep 29, 2021

Thanks for your PR 👍

I've been able to run the most immediately relevant unit tests without issue, but I'm struggling to run tests that depend on a live instance of InfluxDB. How can I configure the tests to run against my InfluxDB server?

You can configure following env variables:

  1. InfluxDB 1.8+ - INFLUXDB_IP, INFLUXDB_PORT_API
  2. InfluxDB 2 - INFLUXDB_2_IP, INFLUXDB_2_PORT

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

Thanks again for your PR 👍

There are a few requirements that must be be satisfy before we accept the PR:

CHANGELOG.md Outdated Show resolved Hide resolved
vesuvian pushed a commit to vesuvian/influxdb-client-csharp that referenced this pull request Sep 29, 2021
vesuvian pushed a commit to vesuvian/influxdb-client-csharp that referenced this pull request Sep 29, 2021
vesuvian pushed a commit to vesuvian/influxdb-client-csharp that referenced this pull request Sep 29, 2021
vesuvian pushed a commit to vesuvian/influxdb-client-csharp that referenced this pull request Sep 29, 2021
@vesuvian
Copy link
Contributor Author

@bednar I am having a miserable time trying to get these tests to run. I've been able to set up my environment variables, I've enabled HTTP in my influxdb.conf, and I am running an instance of InfluxDB 2.0.8

  • Legacy tests fail because of authorization, so I have temporarily hardcoded adding my token to the auth header
  • Now legacy tests fail with a response from the server: "not implemented: CREATE DATABASE" - ok, they're legacy, so I guess I can skip them for v2?
  • LINQ tests that depend on port 9999 immediately fail, so I change it to 8086
  • LINQ tests now fail because ItInfluxDBQueryableTest.SetUp() hardcodes the token as "my-token" - why? should this be an environment variable? Am I meant to do a find and replace?

Is there some documentation I'm missing for how to correctly set up a test environment?

@bednar
Copy link
Contributor

bednar commented Sep 30, 2021

@bednar I am having a miserable time trying to get these tests to run. I've been able to set up my environment variables, I've enabled HTTP in my influxdb.conf, and I am running an instance of InfluxDB 2.0.8

  • Legacy tests fail because of authorization, so I have temporarily hardcoded adding my token to the auth header
  • Now legacy tests fail with a response from the server: "not implemented: CREATE DATABASE" - ok, they're legacy, so I guess I can skip them for v2?
  • LINQ tests that depend on port 9999 immediately fail, so I change it to 8086
  • LINQ tests now fail because ItInfluxDBQueryableTest.SetUp() hardcodes the token as "my-token" - why? should this be an environment variable? Am I meant to do a find and replace?

Is there some documentation I'm missing for how to correctly set up a test environment?

The best way to prepare testing environment is use a Scripts/influxdb-restart.sh script. It will start InfluxDBs and also correctly setup authorization.

vesuvian pushed a commit to vesuvian/influxdb-client-csharp that referenced this pull request Sep 30, 2021
vesuvian pushed a commit to vesuvian/influxdb-client-csharp that referenced this pull request Sep 30, 2021
vesuvian pushed a commit to vesuvian/influxdb-client-csharp that referenced this pull request Sep 30, 2021
vesuvian pushed a commit to vesuvian/influxdb-client-csharp that referenced this pull request Sep 30, 2021
vesuvian pushed a commit to vesuvian/influxdb-client-csharp that referenced this pull request Sep 30, 2021
vesuvian pushed a commit to vesuvian/influxdb-client-csharp that referenced this pull request Sep 30, 2021
@vesuvian
Copy link
Contributor Author

vesuvian commented Sep 30, 2021

@bednar thank you so much for the hand holding. I was ultimately able to set up a debian container with docker, the .net SDK, this repository, and run the influxdb-restart and ci-test.sh scripts with no errors.

I hope you don't mind but I took the liberty of:

  • Removed duplicate entry for PR 239 from the changelog
  • AbstractTest clarifies HTTP request failures, consistent with AbstractTest.WaitToCallback
  • ItTasksApiTest.Runs compares against system time as UTC, previously this test was failing for me

Something still isn't quite right when using linq. My results appear to be correct but the measurement property is not being populated. I'll keep digging and see what I can find.

vesuvian pushed a commit to vesuvian/influxdb-client-csharp that referenced this pull request Sep 30, 2021
vesuvian pushed a commit to vesuvian/influxdb-client-csharp that referenced this pull request Sep 30, 2021
@vesuvian
Copy link
Contributor Author

The measurement column was being explicitly dropped by the QueryAggregator in BuildFluxQuery. I have removed _measurement from the list of dropped columns.

Is this ok by you? I don't want to introduce any performance issues.

vesuvian pushed a commit to vesuvian/influxdb-client-csharp that referenced this pull request Sep 30, 2021
@bednar
Copy link
Contributor

bednar commented Oct 1, 2021

The measurement column was being explicitly dropped by the QueryAggregator in BuildFluxQuery. I have removed _measurement from the list of dropped columns.

Is this ok by you? I don't want to introduce any performance issues.

It is definitely increase amount of data transferred from the server. What do you think about configure this dropping by QueryableOptimizerSettings?

vesuvian pushed a commit to vesuvian/influxdb-client-csharp that referenced this pull request Oct 1, 2021
vesuvian pushed a commit to vesuvian/influxdb-client-csharp that referenced this pull request Oct 1, 2021
@vesuvian
Copy link
Contributor Author

vesuvian commented Oct 1, 2021

I've added optimization settings for dropping start, stop and measurement columns.

At this point I believe I'm done. My application is happily reading/writing to influx using both raw flux and linq queries.

The failing test ItWriteQueryApiTest.Jitter() looks to be entirely unrelated, I'm wondering if there may be a race condition with the timings? I've been able to successfully run all of the tests on my machine.

@bednar
Copy link
Contributor

bednar commented Oct 4, 2021

The failing test ItWriteQueryApiTest.Jitter() looks to be entirely unrelated, I'm wondering if there may be a race condition with the timings?

I don’t think so.

It looks like that ItWriteQueryApiTest.Jitter() is little bit flaky.

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

Thanks again for your PR 👍. We are almost done.

Can you please update a README.md for LINQ driver with information how to use filtering by measurement?

https://github.com/influxdata/influxdb-client-csharp/blob/master/Client.Linq/README.md

vesuvian pushed a commit to vesuvian/influxdb-client-csharp that referenced this pull request Oct 4, 2021
@vesuvian
Copy link
Contributor Author

vesuvian commented Oct 4, 2021

@bednar I've gone ahead and updated the README documents, please let me know if you need anything else

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@bednar bednar merged commit 1d5db86 into influxdata:master Oct 5, 2021
@bednar bednar added this to the 3.1.0 milestone Oct 5, 2021
@bednar bednar added the enhancement New feature or request label Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants