Skip to content

[pkg/translator/azure] Allow timestamp to be used instead of time #28805

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

Conversation

cparkins
Copy link
Contributor

@cparkins cparkins commented Oct 30, 2023

Description:

Allow the attribute 'timestamp' to be used as an alternative to the documented 'time'.

Link to tracking Issue:
[#28806]

Testing:
Using the example from Azure I created a unit test.

Documentation:
Note added to Azure Event Hub Receiver.

@cparkins cparkins requested review from a team October 30, 2023 19:21
@cparkins cparkins changed the title [receiver/azureeventhubs] … [pkg/translator/azure] Allow timestamp to be used instead of time Oct 30, 2023
JSON numbers are encoded as doubles.
* Additionally the field timestamp can be used instead of time.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Additionally the field timestamp can be used instead of time.
* Additionally the field `timestamp` can be used instead of `time`.

Formatting nit.

Do you want to add the timestamp/timeStamp field to the table above since it's supported now?

Also, would it be helpful to state in the README here that if both are included, time takes precedence over timestamp?

Copy link
Contributor Author

@cparkins cparkins Oct 30, 2023

Choose a reason for hiding this comment

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

I added the note as suggested. I was trying to make the least amount of change to the table above to avoid making it confusing. Hopefully what I did makes sense.

@@ -95,10 +95,11 @@ and the OpenTelemetry attributes.
| resultSignature (optional) | azure.result.signature (attribute) |
| resultType (optional) | azure.result.type (attribute) |
| tenantId (required, tenant logs) | azure.tenant.id (attribute) |
| time (required) | time_unix_nano (field) |
| time or timeStamp (required) | time_unix_nano (time takes precedence) |
Copy link
Member

@crobert-1 crobert-1 Oct 30, 2023

Choose a reason for hiding this comment

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

It would make more sense to me to have the time takes precedence comment in the same column as time or timeStamp, but this is just personal preference, this should be generally understandable.

@sigurdfalk
Copy link

@atoulme @djaglowski would it be possible to get some reviews on this one? We are waiting eagerly on this one, would be very appreciated 😊

@sigurdfalk
Copy link

@bogdandrutu Possible to get this one into next release? 🙏🏻

cparkins added a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Jan 10, 2024
…en-telemetry#28805)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->

Allow the attribute 'timestamp' to be used as an alternative to the
documented 'time'.

**Link to tracking Issue:** <Issue number if applicable>
[open-telemetry#28806]

**Testing:** <Describe what testing was performed and which tests were
added.>
Using the example from Azure I created a unit test.

**Documentation:** <Describe the documentation added.>
Note added to Azure Event Hub Receiver.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants