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

issue-5230 #455

Closed

Conversation

be-a-bee
Copy link
Contributor

added traceid and spanid to decision log.

test for checking traceid in decision log is not passing

added traceid and spanid to decision log.

test for checking traceid in decision log is not passing
added traceid and spanid to decision log.

test for checking traceid in decision log is not passing
Copy link
Member

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @LionOnTheChase. Few comments inline.

info.SpanID = sctx.SpanID().String()
}

p.manager.Logger().WithFields(map[string]interface{}{
Copy link
Member

Choose a reason for hiding this comment

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

We should remove this log as much of this is already logged at debug level in the check call. This is also resulting in the test failure.

@@ -165,6 +167,38 @@ func TestCheckAllow(t *testing.T) {
}
}

func TestCheckTraceIDInAllowedRequest(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

You could use something like this as a reference to setup the unit test.

@ashutosh-narkar
Copy link
Member

Closing this for now. Feel free to pick this back up and address the comments when you get a chance. Thanks for looking into this.

@be-a-bee
Copy link
Contributor Author

@ashutosh-narkar , thank you for your patience. I have addressed your comments and created a new PR #462 .

Kindly review

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.

2 participants