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

Adding fields 'aws_log_group' and 'aws_log_stream' to flow log and raw log codecs #55

Merged
merged 2 commits into from Nov 9, 2017

Conversation

@danielgrant
Copy link
Contributor

@danielgrant danielgrant commented Nov 2, 2017

This pull request addresses #54.

@CLAassistant
Copy link

@CLAassistant CLAassistant commented Nov 2, 2017

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@joschi joschi left a comment

Could you please add some tests for the new fields?

@@ -51,6 +51,8 @@ public Message decodeLogData(@Nonnull final CloudWatchLogEvent logEvent) {
flowLogMessage.getTimestamp()
);
result.addFields(buildFields(flowLogMessage));
result.addField("aws_log_group", logGroup);

This comment has been minimized.

@joschi

joschi Nov 9, 2017
Contributor

Please extract the key names as constants into the existing AWS class and use them in CloudWatchFlowLogCodec and in CloudWatchRawLogCodec.

This comment has been minimized.

@danielgrant

danielgrant Nov 9, 2017
Author Contributor

I've extracted these fields to constants.

@JsonProperty("logEvents")
public List<CloudWatchLogEvent> logEvents;

@JsonProperty("logGroup")
public String logGroup;

This comment has been minimized.

@joschi

joschi Nov 9, 2017
Contributor

Can this ever by null (i. e. not exist)?

This comment has been minimized.

@danielgrant

danielgrant Nov 9, 2017
Author Contributor

Looking at the documentation and example, there is nothing to suggest that this field is optional / might not be present in any situation. Additionally, in all of my testing to date it has never not been present.

This comment has been minimized.

@joschi

joschi Nov 9, 2017
Contributor

public String logGroup;

@JsonProperty("logStream")
public String logStream;

This comment has been minimized.

@joschi

joschi Nov 9, 2017
Contributor

Can this ever by null (i. e. not exist)?

This comment has been minimized.

@danielgrant

danielgrant Nov 9, 2017
Author Contributor

Looking at the documentation and example, there is nothing to suggest that this field is optional / might not be present in any situation. Additionally, in all of my testing to date it has never not been present.

This comment has been minimized.

@joschi

joschi Nov 9, 2017
Contributor

@danielgrant
Copy link
Contributor Author

@danielgrant danielgrant commented Nov 9, 2017

Hi @joschi,

Thanks for the feedback.

With regards to the unit testing, I couldn't see any existing test classes / unit testing around the classes that I've modified when I was making these changes. I can build these from scratch, but other priorities elsewhere means that doing so will take a bit longer.

Cheers,
Daniel.

@joschi
joschi approved these changes Nov 9, 2017
@JsonProperty("logEvents")
public List<CloudWatchLogEvent> logEvents;

@JsonProperty("logGroup")
public String logGroup;

This comment has been minimized.

@joschi

joschi Nov 9, 2017
Contributor

public String logGroup;

@JsonProperty("logStream")
public String logStream;

This comment has been minimized.

@joschi

joschi Nov 9, 2017
Contributor

@joschi joschi self-assigned this Nov 9, 2017
@joschi joschi added this to the 3.0.0 milestone Nov 9, 2017
@joschi joschi merged commit 8e53cca into Graylog2:master Nov 9, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@garybot2
graylog-project/pr Jenkins build graylog-project-pr-snapshot 678 has succeeded
Details
license/cla Contributor License Agreement is signed.
Details
joschi pushed a commit that referenced this pull request Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants