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 specification for EventLoggerProvider and EventLogger #4031

Merged
merged 20 commits into from
May 23, 2024

Conversation

martinkuba
Copy link
Contributor

@martinkuba martinkuba commented May 3, 2024

This adds details to how EventLoggerProvider and EventLogger should be implemented in the default Events SDK.

Prototypes

Copy link
Contributor

@tedsuo tedsuo left a comment

Choose a reason for hiding this comment

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

looks great. 👍

Copy link
Member

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

A few nits but otherwise LGTM !

specification/logs/event-sdk.md Outdated Show resolved Hide resolved
specification/logs/event-sdk.md Outdated Show resolved Hide resolved
specification/logs/event-sdk.md Outdated Show resolved Hide resolved
@JamieDanielson
Copy link
Member

(some lint checks to fix up but content matches what we discussed)

@martinkuba martinkuba marked this pull request as ready for review May 3, 2024 21:19
@martinkuba martinkuba requested review from a team as code owners May 3, 2024 21:19
@martinkuba martinkuba changed the title Add specification to EventLoggerProvider and EventLogger Add specification for EventLoggerProvider and EventLogger May 3, 2024
specification/logs/event-sdk.md Show resolved Hide resolved
specification/logs/event-sdk.md Outdated Show resolved Hide resolved
specification/logs/event-sdk.md Outdated Show resolved Hide resolved
specification/logs/event-sdk.md Outdated Show resolved Hide resolved
Copy link
Contributor

@breedx-splk breedx-splk 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, just thought it might be helpful to indicate if the two methods are required or optional. Thanks!

specification/logs/event-sdk.md Outdated Show resolved Hide resolved
specification/logs/event-sdk.md Show resolved Hide resolved
@carlosalberto
Copy link
Contributor

@martinkuba Also add a CHANGELOG entry please ;)

@carlosalberto
Copy link
Contributor

@tsloughter @MSNev any last comments? Otherwise we are good to go.

Copy link

linux-foundation-easycla bot commented May 21, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@reyang
Copy link
Member

reyang commented May 21, 2024

CLA failed @martinkuba :

image

@martinkuba
Copy link
Contributor Author

@JamieDanielson Can you please sign the CLA? Or, I can rebase the commits and change the author.

@JamieDanielson
Copy link
Member

JamieDanielson commented May 22, 2024

@JamieDanielson Can you please sign the CLA? Or, I can rebase the commits and change the author.

Oh weird I guess it updated my default email in GitHub? Co-authored-by: Jamie Danielson [email protected] can you replace with Co-authored-by: Jamie Danielson [email protected]? should be good to go now, sorry about that.

@JamieDanielson
Copy link
Member

/easycla

@trask
Copy link
Member

trask commented May 22, 2024

Oh weird, I guess it updated my default email in GitHub? Co-authored-by: Jamie Danielson [email protected] can you replace with Co-authored-by: Jamie Danielson [email protected]? should be good to go now, sorry about that.

@JamieDanielson we've seen this several times over the past week or so https://cloud-native.slack.com/archives/C01NJ7V1KRC/p1716378324118029, what did you do to fix it? 🪄

@JamieDanielson
Copy link
Member

Oh weird, I guess it updated my default email in GitHub? Co-authored-by: Jamie Danielson [email protected] can you replace with Co-authored-by: Jamie Danielson [email protected]? should be good to go now, sorry about that.

@JamieDanielson we've seen this several times over the past week or so https://cloud-native.slack.com/archives/C01NJ7V1KRC/p1716378324118029, what did you do to fix it? 🪄

Seems like there was a bug. An update came through on the linked issue suggesting to comment with /easycla and that seems to have worked 🎉

Update:
We have identified the root cause of the issue and decided to rollback the co-author changes to unblock the community, since the fix will take longer engineering time. The PR should update by adding /easycla comment, which I added the in the reported PRs and confirmed EasyCLA passing.
Thank you for your patience.

@carlosalberto carlosalberto merged commit e11567a into open-telemetry:main May 23, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants