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

Limit list_log_file response size #265

Merged
merged 2 commits into from
Aug 1, 2018
Merged

Limit list_log_file response size #265

merged 2 commits into from
Aug 1, 2018

Conversation

mariussturm
Copy link
Contributor

@mariussturm mariussturm added this to the 3.0.0 milestone Jul 19, 2018
@mariussturm mariussturm requested a review from bernd July 19, 2018 13:23
api/graylog.go Outdated
@@ -151,7 +152,16 @@ func UpdateRegistration(httpClient *http.Client, ctx *context.Ctx, status *grayl
registration.NodeDetails.Status = status
registration.NodeDetails.Metrics = metrics
if len(ctx.UserConfig.ListLogFiles) > 0 {
registration.NodeDetails.LogFileList = common.ListFiles(ctx.UserConfig.ListLogFiles)
fileList := common.ListFiles(ctx.UserConfig.ListLogFiles)
fileListSize := (int)(unsafe.Sizeof(common.File{})) * len(fileList)
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually correct? As far as I understand, this measures the size of an empty common.File struct. But a filled struct might be much larger because we don't know the length of the strings beforehand.

How about measuring the serialized request by marshaling it into JSON?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was thinking about that too, my concern was that we would do the same operation two times. Marshal to JSON to get the list size... marshal again to actually send the request. It's not too expensive but anyway. Guess the result will be better because we also get the JSON overhead.

Copy link
Member

@bernd bernd left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@bernd bernd merged commit 1a6ca34 into master Aug 1, 2018
@bernd bernd self-assigned this Aug 1, 2018
@bernd bernd deleted the limit-file-list branch August 1, 2018 11:59
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.

2 participants