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

[exporter/clickhouse] use observed timestamp in logs #34151

Conversation

pimeys
Copy link
Contributor

@pimeys pimeys commented Jul 18, 2024

Description: Some senders, such as the rust-opentelemetry do not send timestamp in their logs. In these cases, if the timestamp is not set, we must use the observed timestamp instead.

Link to tracking Issue: #34150

Testing: Added a test which does not define timestamp

@pimeys pimeys requested a review from dmitryax as a code owner July 18, 2024 09:41
@pimeys pimeys requested a review from a team July 18, 2024 09:41
Copy link

linux-foundation-easycla bot commented Jul 18, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: pimeys / name: Julius de Bruijn (56cceb8)
  • ✅ login: crobert-1 / name: Curtis Robert (8ba3fe5, 7f299a8)
  • ✅ login: mx-psi / name: Pablo Baeyens (a7c8ac9)
  • ✅ login: codeboten / name: Alex Boten (d9b677f)

Copy link
Member

@hanjm hanjm left a comment

Choose a reason for hiding this comment

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

code LGTM, Thanks. please add changelog.

Copy link
Member

@SpencerTorres SpencerTorres left a comment

Choose a reason for hiding this comment

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

Nice find, appreciate the contribution. Add a changelog entry and this should be good to go.

Thanks!

@pimeys pimeys force-pushed the use-observed-timestamp-if-timestamp-is-missing branch 2 times, most recently from d1872aa to 434e0b4 Compare July 22, 2024 06:57
@pimeys
Copy link
Contributor Author

pimeys commented Jul 22, 2024

I added the changelog and amended. Should be good now.

@pimeys pimeys force-pushed the use-observed-timestamp-if-timestamp-is-missing branch 2 times, most recently from d716293 to ca2f6bb Compare July 25, 2024 13:41
…missing

Some senders, such as the rust-opentelemetry do _not_ send timestamp
in their logs. In these cases, if the timestamp is not set, we must
use the observed timestamp instead.
@pimeys pimeys force-pushed the use-observed-timestamp-if-timestamp-is-missing branch from ca2f6bb to 56cceb8 Compare July 26, 2024 18:26
@SpencerTorres
Copy link
Member

cc @dmitryax @crobert-1

@SpencerTorres
Copy link
Member

@pimeys looks like there's a couple nits to the changelog. Let me know if you can resolve those so we can get this merged 👍

@crobert-1 crobert-1 added the ready to merge Code review completed; ready to merge by maintainers label Jul 31, 2024
@codeboten codeboten merged commit 6d8e6f6 into open-telemetry:main Aug 7, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Aug 7, 2024
f7o pushed a commit to f7o/opentelemetry-collector-contrib that referenced this pull request Sep 12, 2024
…34151)

**Description:** Some senders, such as the rust-opentelemetry do _not_
send timestamp in their logs. In these cases, if the timestamp is not
set, we must use the observed timestamp instead.

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

**Testing:** Added a test which does not define timestamp

---------

Co-authored-by: Curtis Robert <[email protected]>
Co-authored-by: Pablo Baeyens <[email protected]>
Co-authored-by: Alex Boten <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter/clickhouse ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants