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

[receiver/sqlqueryreceiver] Fix tracking results by timestamp column #35195

Conversation

Grandys
Copy link
Contributor

@Grandys Grandys commented Sep 15, 2024

Description:

Formats retrieved time columns with milliseconds precision, so they are not reprocessed when used as a tracking_column

Link to tracking Issue: #35194

Testing: Added integration test, updated test data

Documentation: n/a

Closes #35194

Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

One minor nit, but it looks good. Thanks for making this change!

receiver/sqlqueryreceiver/integration_test.go Outdated Show resolved Hide resolved
internal/sqlquery/row_scanner.go Show resolved Hide resolved
@Grandys Grandys requested a review from a team as a code owner September 18, 2024 20:37
@Grandys
Copy link
Contributor Author

Grandys commented Sep 18, 2024

One minor nit, but it looks good. Thanks for making this change!

@crobert-1
You're welcome.
Do you want to wait for a second reviewer, or you'll mark it as ready to merge ? Same question for #35189

@crobert-1 crobert-1 added the ready to merge Code review completed; ready to merge by maintainers label Sep 18, 2024
Copy link
Member

@andrzej-stencel andrzej-stencel left a comment

Choose a reason for hiding this comment

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

Thank you for this fix @Grandys.

I'm only worried that we don't have test coverage for logs tracking for database engines other than PostgreSQL. This means that we're treading in the dark with this feature and changes to it like this valuable fix. As a result, there's a risk that this fix breaks the logs tracking feature for one of the untested engines (e.g. because the RFC3939Nano format is not supported).

I'm supportive of merging this fix, given that the logs stability is currently development. We should have integrations tests for all supported database engines before we move logs support out of development into alpha stability or further.

@andrzej-stencel andrzej-stencel merged commit 4c6d6a5 into open-telemetry:main Sep 30, 2024
162 of 163 checks passed
@github-actions github-actions bot added this to the next release milestone Sep 30, 2024
jriguera pushed a commit to springernature/opentelemetry-collector-contrib that referenced this pull request Oct 4, 2024
…pen-telemetry#35195)

**Description:**

Formats retrieved time columns with milliseconds precision, so they are
not reprocessed when used as a tracking_column

**Link to tracking Issue:** open-telemetry#35194

**Testing:** Added integration test, updated test data

**Documentation:** n/a

Closes open-telemetry#35194
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal/sqlquery ready to merge Code review completed; ready to merge by maintainers receiver/sqlquery SQL query receiver
Projects
None yet
4 participants