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

not print successfully if there are Unprocessed segments #64

Merged
merged 1 commit into from
Aug 13, 2020
Merged

not print successfully if there are Unprocessed segments #64

merged 1 commit into from
Aug 13, 2020

Conversation

wangzlei
Copy link
Contributor

Issue #, if available:
#63

Description of changes:
If part of segments break the x-ray rules like invalid ID, at present the log is:

2020-08-11T22:59:59-07:00 [Info] Successfully sent batch of 2 segments (0.064 seconds)

This fix improves log detail, if there are Unprocessed segments returned, print:

2020-08-11T23:00:15-07:00 [Info] Sent batch of 2 segments but had 1 Unprocessed segments (0.080 seconds)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@wangzlei wangzlei changed the title not print if there are Unprocessed segments not print successfully if there are Unprocessed segments Aug 12, 2020
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment on lines +100 to +102
for i := 0; i < len(batch); i++ {
log.Debug(*batch[i])
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this still N^2 logging? E.g. in the case of an entire batch of 50 segments being unprocessed, I believe this would still print out 52 messages/segment = 2,600 messages total.

IMO we either don't need to print out the entire batch ever, or we should just do it once outside the UnprocessedTraceSegments loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We just print the Unprocessed segments here, see the test result:

2020-08-11T23:00:00-07:00 [Info] Successfully sent batch of 2 segments (0.077 seconds)
2020-08-11T23:00:01-07:00 [Info] Successfully sent batch of 2 segments (0.076 seconds)
2020-08-11T23:00:02-07:00 [Info] Successfully sent batch of 2 segments (0.081 seconds)
2020-08-11T23:00:03-07:00 [Info] Successfully sent batch of 2 segments (0.066 seconds)
2020-08-11T23:00:11-07:00 [Info] Successfully sent batch of 1 segments (0.067 seconds)
2020-08-11T23:00:12-07:00 [Info] Sent batch of 2 segments but had 1 Unprocessed segments (0.088 seconds)
2020-08-11T23:00:12-07:00 [Error] Unprocessed segment: {
  ErrorCode: "InvalidId",
  Id: "0",
  Message: "Invalid segment. ErrorCode: InvalidId"
}
2020-08-11T23:00:13-07:00 [Info] Sent batch of 2 segments but had 1 Unprocessed segments (0.063 seconds)
2020-08-11T23:00:13-07:00 [Error] Unprocessed segment: {
  ErrorCode: "InvalidId",
  Id: "0",
  Message: "Invalid segment. ErrorCode: InvalidId"
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Is debug level logging enabled in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, will fix that in #62

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR I think is just a small one to improve the log message (not always say Successfully), it's not a fix for the n^2 logging @willarmiros

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, was confused by the closure of #63

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

3 participants