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 performance test for filelog based kubernetes container logs receiver #2564

Merged

Conversation

sumo-drosiek
Copy link
Member

Add performance test for filelog based kubernetes container logs receiver

This PR contains one performance test (for kubernetes containerd logs with format autodetection)

Link to tracking Issue:
#2266

Testing:
Perf tests

Documentation:
N/A

@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #2564 (c945b11) into main (a7fbb60) will increase coverage by 22.13%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2564       +/-   ##
===========================================
+ Coverage   69.18%   91.31%   +22.13%     
===========================================
  Files          34      429      +395     
  Lines        1603    21457    +19854     
===========================================
+ Hits         1109    19594    +18485     
- Misses        420     1394      +974     
- Partials       74      469      +395     
Flag Coverage Δ
integration 69.18% <ø> (ø)
unit 90.22% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
receiver/receivercreator/receiver.go 66.66% <0.00%> (ø)
...cedetectionprocessor/internal/resourcedetection.go 97.05% <0.00%> (ø)
...nalfxexporter/translation/logdata_to_signalfxv2.go 82.75% <0.00%> (ø)
exporter/dynatraceexporter/metrics_exporter.go 86.27% <0.00%> (ø)
receiver/memcachedreceiver/scraper.go 15.15% <0.00%> (ø)
exporter/awsxrayexporter/factory.go 100.00% <0.00%> (ø)
receiver/kubeletstatsreceiver/kubelet/cert.go 63.63% <0.00%> (ø)
...ctionprocessor/internal/aws/elasticbeanstalk/fs.go 0.00% <0.00%> (ø)
...er/awsemfexporter/mapwithexpiry/map_with_expiry.go 100.00% <0.00%> (ø)
...ver/k8sclusterreceiver/collection/metadatastore.go 0.00% <0.00%> (ø)
... and 407 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7fbb60...c945b11. Read the comment docs.

testbed/datasenders/k8s.go Outdated Show resolved Hide resolved
testbed/datasenders/k8s.go Outdated Show resolved Hide resolved

// NewFileLogK8sWriter creates a new data sender that will write kubernetes containerd
// log entries to a file, to be tailed by FileLogReceiver and sent to the collector.
func NewFileLogK8sWriter(config string) *FileLogK8sWriter {
Copy link
Member

Choose a reason for hiding this comment

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

Add comment to explain what config must contain.

@@ -31,4 +34,42 @@ func TestMain(m *testing.M) {
testbed.DoTestMain(m, contribPerfResultsSummary)
}

// This file is left with no tests as a placeholder for future unstable exe tests
func TestLog10kDPS(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.

Can we move this to the stable log_test.go instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, I was not sure what exactly unstable means

Copy link
Member

Choose a reason for hiding this comment

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

The tests_unstable_exe/log_test.go is for testing the unstable executable.
Stanza/filelog receiver previously was part of unstable executable. It was moved to stable exe yesterday.

@sumo-drosiek sumo-drosiek force-pushed the drosiek-stanza-transform-ops branch 4 times, most recently from 02a5e52 to 339dc66 Compare March 5, 2021 16:10
@sumo-drosiek
Copy link
Member Author

@tigrannajaryan All fixed, but Ci seems to be unstable Today

@tigrannajaryan
Copy link
Member

I think there is a linting error (in addition to failing load test, which is probably CircleCI fault).

@jrcamp jrcamp assigned tigrannajaryan and unassigned jrcamp Mar 5, 2021
@sumo-drosiek
Copy link
Member Author

sumo-drosiek commented Mar 8, 2021

@tigrannajaryan I doubt this lint error related to this PR, as all recent PRs have same issue eg. https://app.circleci.com/pipelines/github/open-telemetry/opentelemetry-collector-contrib/10631/workflows/5e23abf9-f769-4d99-908e-c046ea55bbda/jobs/89573

make[1]: Entering directory '/home/circleci/project'
golangci-lint run --allow-parallel-runners
ERRO Running error: context loading failed: no go files to analyze 
make[1]: *** [Makefile.Common:102: lint] Error 5

@sumo-drosiek
Copy link
Member Author

@tigrannajaryan I added more tests in this PR and example is available in PR #2606

@tigrannajaryan
Copy link
Member

@tigrannajaryan I doubt this lint error related to this PR, as all recent PRs have same issue eg. https://app.circleci.com/pipelines/github/open-telemetry/opentelemetry-collector-contrib/10631/workflows/5e23abf9-f769-4d99-908e-c046ea55bbda/jobs/89573

make[1]: Entering directory '/home/circleci/project'
golangci-lint run --allow-parallel-runners
ERRO Running error: context loading failed: no go files to analyze 
make[1]: *** [Makefile.Common:102: lint] Error 5

Rebasing from main should help. It was broken.

// NewFileLogK8sWriter creates a new data sender that will write kubernetes containerd
// log entries to a file, to be tailed by FileLogReceiver and sent to the collector.
//
// config is an OTC config appended to the receivers section after executing fmt.Sprintf on it.
Copy link
Member

Choose a reason for hiding this comment

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

What is OTC? If it refers to collector just use Collector or Otel Collector or Otelcol. We don't normally use OTC.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will use Otelcol then

testbed/datasenders/k8s.go Outdated Show resolved Hide resolved
@djaglowski
Copy link
Member

djaglowski commented Mar 8, 2021

@sumo-drosiek The latest release of opentelemetry-log-collection will require some minor changes to this code.

This should be a very straightforward change from using the term labels to attributes.

@sumo-drosiek sumo-drosiek force-pushed the drosiek-stanza-transform-ops branch 2 times, most recently from 2662e93 to c75f8ac Compare March 8, 2021 20:52
Dominik Rosiek and others added 5 commits March 8, 2021 22:02
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
@sumo-drosiek sumo-drosiek force-pushed the drosiek-stanza-transform-ops branch from c75f8ac to c945b11 Compare March 8, 2021 21:02
_ "github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/parser/json"
_ "github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/parser/regex"
_ "github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/parser/severity"
_ "github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/parser/time"
_ "github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/transformer/metadata"
_ "github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/transformer/restructure"
_ "github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/transformer/router"
Copy link
Member Author

Choose a reason for hiding this comment

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

I was unable to make linter happy with comment above transformer section. At least one of two linters has been always raising some issues:

Check License finished successfully
internal/stanza/register.go: Import groups are not in the proper order: ["Third party" "Third party"]

or

register.go:22: File is not `goimports`-ed with -local github.com/open-telemetry/opentelemetry-collector-contrib (goimports)
        _ "github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/parser/time"

or

register.go:23: File is not `gofmt`-ed with `-s` (gofmt)

I tried three different approaches

	_ "github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/parser/time"
	// comment here
	_ "github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/transformer/metadata"
	_ "github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/parser/time"

	// comment here
	_ "github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/transformer/metadata"
	_ "github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/parser/time"
        
	// comment here (preceding line is tab indented)
	_ "github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/transformer/metadata"

Copy link
Member

Choose a reason for hiding this comment

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

No need to split into two groups - looks good to me like this.

@sumo-drosiek
Copy link
Member Author

@tigrannajaryan @djaglowski Everything addressed, CI is green :)

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

LGTM

@tigrannajaryan tigrannajaryan merged commit ccc3f95 into open-telemetry:main Mar 9, 2021
@tigrannajaryan
Copy link
Member

Thank you @sumo-drosiek

kisieland referenced this pull request in kisieland/opentelemetry-collector-contrib Mar 16, 2021
Ensure that the receiver finishes all ongoing requests, and does not accept any new request after shutdown is finished.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
tigrannajaryan pushed a commit that referenced this pull request Mar 16, 2021
**Description:** 
This PR adds example configuration to handle kubernetes container logs using filelog receiver
This depends on the #2564

**Link to tracking Issue:**
#2266 

**Testing:**
N/A
tests which were performed to get results are going to be issued in another PR

**Documentation:**
README.md
pmatyjasek-sumo referenced this pull request in pmatyjasek-sumo/opentelemetry-collector-contrib Apr 28, 2021
…iver (#2564)

Add performance test for filelog based kubernetes container logs receiver

This PR contains one performance test (for kubernetes containerd logs with format autodetection)

**Link to tracking Issue:**
#2266
pmatyjasek-sumo referenced this pull request in pmatyjasek-sumo/opentelemetry-collector-contrib Apr 28, 2021
**Description:** 
This PR adds example configuration to handle kubernetes container logs using filelog receiver
This depends on the #2564

**Link to tracking Issue:**
#2266 

**Testing:**
N/A
tests which were performed to get results are going to be issued in another PR

**Documentation:**
README.md
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.

4 participants