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

add mongodb OP_MSG (2013) #8594

Closed
wants to merge 23 commits into from
Closed

add mongodb OP_MSG (2013) #8594

wants to merge 23 commits into from

Conversation

pohzipohzi
Copy link

this fixes #6191

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@pohzipohzi pohzipohzi requested a review from a team as a code owner January 5, 2019 08:47
@elasticcla
Copy link

Hi @pohzipohzi, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

1 similar comment
@elasticcla
Copy link

Hi @pohzipohzi, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@pohzipohzi
Copy link
Author

@adriansr @cwurm any update on this?

@adriansr
Copy link
Contributor

adriansr commented Jan 8, 2019

It looks good. However, the issue is the same as with #6440, we would like to have some basic tests for this.

Something like loading a PCAP with the new opcodes and checking that the emitted events are fine. There's a lot of examples in packetbeat, like this one https://github.com/elastic/beats/blob/master/packetbeat/tests/system/test_0001_mysql_spaces.py

@adriansr
Copy link
Contributor

adriansr commented Feb 18, 2019

@pohzipohzi will you be able to add that test?

@adriansr adriansr self-assigned this Feb 18, 2019
@@ -106,7 +106,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d

*Packetbeat*

- Add support for mongodb opcode 2013 (OP_MSG). {issue}6191[6191] {pull}8594[8594]
Copy link
Contributor

@adriansr adriansr Feb 18, 2019

Choose a reason for hiding this comment

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

You've lost your changes to CHANGELOG.asciidoc while merging.

Note that now we use CHANGELOG.next.asciidoc for unreleased functionality, so you should add it there.

@pohzipohzi
Copy link
Author

@adriansr sorry for taking so long to get back to you. Honestly I haven't touched this project in some time and I'm having some trouble setting up my environment to run python related tests. I will be a little busy this week but I do intend to add the test by next week or so

@pohzipohzi pohzipohzi requested a review from a team as a code owner February 25, 2019 16:09
@pohzipohzi pohzipohzi requested review from a team as code owners March 27, 2019 23:53
@@ -38,7 +38,7 @@ type flowsProcessor struct {
}

var (
ErrInvalidTimeout = errors.New("timeout must not <= 1s")
ErrInvalidTimeout = errors.New("timeout must be >= 1s")

Choose a reason for hiding this comment

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

exported var ErrInvalidTimeout should have comment or be unexported

@@ -96,22 +100,22 @@ func TestFetchEventContents(t *testing.T) {
assert.EqualValues(t, 2872860672, pgInfo["used_bytes"])

//check pg_state info
pg_stateInfo := events[1]["pg_state"].(common.MapStr)
pg_stateInfo := events[1].MetricSetFields["pg_state"].(common.MapStr)

Choose a reason for hiding this comment

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

don't use underscores in Go names; var pg_stateInfo should be pgStateInfo


// +build integration

package s3_request

Choose a reason for hiding this comment

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

don't use an underscore in package name

// or more contributor license agreements. Licensed under the Elastic License;
// you may not use this file except in compliance with the Elastic License.

package s3_request

Choose a reason for hiding this comment

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

don't use an underscore in package name

// or more contributor license agreements. Licensed under the Elastic License;
// you may not use this file except in compliance with the Elastic License.

package s3_request

Choose a reason for hiding this comment

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

don't use an underscore in package name

@@ -20,12 +20,20 @@ type Field struct {

type FieldDict map[Key]*Field

func RegisterFields(dict FieldDict) error {
func RegisterGlobalFields(dict FieldDict) error {

Choose a reason for hiding this comment

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

exported function RegisterGlobalFields should have comment or be unexported

@@ -6,7 +6,7 @@ package fields

import "fmt"

var Fields = FieldDict{}
var GlobalFields = FieldDict{}

Choose a reason for hiding this comment

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

exported var GlobalFields should have comment or be unexported


// WithBeatMeta adds beat meta information as builtin fields to a processing pipeline.
// The `key` parameter defines the field to be used.
func WithBeatMeta(key string) modifier {

Choose a reason for hiding this comment

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

exported func WithBeatMeta returns unexported type processing.modifier, which can be annoying to use

}

// WithECS modifier adds `ecs.version` builtin fields to a processing pipeline.
var WithECS modifier = WithFields(common.MapStr{

Choose a reason for hiding this comment

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

should omit type modifier from declaration of var WithECS; it will be inferred from the right-hand side

}

// WithFields creates a modifier with the given default builtin fields.
func WithFields(fields common.MapStr) modifier {

Choose a reason for hiding this comment

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

exported func WithFields returns unexported type processing.modifier, which can be annoying to use

@pohzipohzi
Copy link
Author

Closing and continuing in #11500

@pohzipohzi pohzipohzi closed this Mar 28, 2019
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.

Update MongoDB protocol with new opcodes
5 participants