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

Use logtest.AssertRecordEqual in otellogrus test #5851

Closed
MrAlias opened this issue Jul 3, 2024 · 3 comments · Fixed by #5852
Closed

Use logtest.AssertRecordEqual in otellogrus test #5851

MrAlias opened this issue Jul 3, 2024 · 3 comments · Fixed by #5852
Assignees
Milestone

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Jul 3, 2024

Originally posted by @pellared in #5847 (comment)

@MrAlias
Copy link
Contributor Author

MrAlias commented Jul 3, 2024

This is not immediately resolvable. There is an impedance mismatch.

AssertRecordEqual accepts log.Records. But the recorder used in the mentioned test produces logtest.EmittedRecords. An AssertEmittedRecordEqual should be added or the EmittedRecord return for a recorder needs to be reconsidered in logtest.

cc @dmathieu

@MrAlias MrAlias added the blocked: opentelemetry-go Waiting on changes to or a release of the opentelemetry-go project label Jul 3, 2024
@dmathieu
Copy link
Member

dmathieu commented Jul 3, 2024

I solved that problem in #5852 by using j.Record, which is made available by EmittedRecord.

@dmathieu
Copy link
Member

dmathieu commented Jul 3, 2024

the EmittedRecord return for a recorder needs to be reconsidered in logtest.

We need EmittedRecord if we want to keep storing the context.

An AssertEmittedRecordEqual should be added

That assertion would probably have to check for the context as well, which we wouldn't want to do in the logrus bridge anyway.

@MrAlias MrAlias removed the blocked: opentelemetry-go Waiting on changes to or a release of the opentelemetry-go project label Jul 3, 2024
@MrAlias MrAlias added this to the v1.29.0 milestone Aug 7, 2024
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 a pull request may close this issue.

2 participants