Add lifecycle tests to all default components #2741

pjanotti · 2021-03-19T21:12:28Z

Why

Add lifetime tests to remaining default components in line with #2679. This allows changing the components without restarting the process.

What

Description:
Separate the default components test per type of component and also allow to run the test for a single component. Some receivers are not being tested for lifecycle in this PR:

Zipkin and OTLP receiver (I will have PRs to fix these soon)
Kafka receiver: requires access to the internals of the receiver to perform a successful Start call
OpenCensus: can't be properly stopped due to usage CMux (the CMux listener Accept calls block forever)

codecov · 2021-03-19T21:20:20Z

Codecov Report

Merging #2741 (b651d96) into main (76237c7) will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2741      +/-   ##
==========================================
+ Coverage   91.77%   91.81%   +0.04%     
==========================================
  Files         291      291              
  Lines       15535    15535              
==========================================
+ Hits        14257    14264       +7     
+ Misses        877      870       -7     
  Partials      401      401

Impacted Files	Coverage Δ
...babilisticsamplerprocessor/probabilisticsampler.go	`98.98% <0.00%> (+2.02%)`	⬆️
receiver/prometheusreceiver/factory.go	`81.81% <0.00%> (+3.03%)`	⬆️
exporter/fileexporter/file_exporter.go	`84.61% <0.00%> (+7.69%)`	⬆️
exporter/fileexporter/factory.go	`100.00% <0.00%> (+11.53%)`	⬆️

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 76237c7...b651d96. Read the comment docs.

tigrannajaryan

Nice. So this uncovered some bugs.

service/defaultcomponents/default_receivers_test.go

bogdandrutu · 2021-03-20T00:51:07Z

@tigrannajaryan bugs based on? I think we need first to document the requirements before claiming that something is a bug :)

tigrannajaryan · 2021-03-20T03:32:26Z

@tigrannajaryan bugs based on? I think we need first to document the requirements before claiming that something is a bug :)

@bogdandrutu My understanding from the skipped tests is that some components fail the Create/Start/Shutdown/Create/Start/Shutdown sequence (did I misunderstand it?).

I think we can consider that a bug, given our discussions about how we should be able to restart the Collector with a new config. Sure, let's document the requirement clearly.

bogdandrutu · 2021-03-20T14:55:19Z

Not sure if I would call missunderstand but the sequence that is tested is Create/Start/Create/Shutdown/Start/Shutdown so let's document exactly what is the expectations so I can ensure that we test what is expected.

I understand the need to be able to restart the collector, but let's document that and then we can push all these PRs

tigrannajaryan · 2021-03-22T13:46:35Z

Not sure if I would call missunderstand but the sequence that is tested is Create/Start/Create/Shutdown/Start/Shutdown so let's document exactly what is the expectations so I can ensure that we test what is expected.

I understand the need to be able to restart the collector, but let's document that and then we can push all these PRs

@pjanotti I think @bogdandrutu is right, the test verifies the sequence where we create the second instance before we shutdown the first. I think the actual sequence we need is Create/Start/Shutdown/Create/Start/Shutdown, right? It should be easy to change the test to do this instead.

service/defaultcomponents/default_exporters_test.go

bogdandrutu · 2021-03-22T18:18:16Z

Please update tests accordingly to #2757

bogdandrutu · 2021-03-22T20:10:19Z

service/defaultcomponents/default_exporters_test.go

+
+		secondExp, err := createFn(ctx, expCreateParams, getConfigFn())
+		require.NoError(t, err)
+


Remove empty line.

bogdandrutu · 2021-03-22T20:13:09Z

service/defaultcomponents/default_exporters_test.go

+		if err == configerror.ErrDataTypeIsNotSupported {
+			continue
+		}


if errors.Is(err, configerror.ErrDataTypeIsNotSupported)

bogdandrutu · 2021-03-22T20:15:00Z

service/defaultcomponents/default_extensions_test.go


 	secondExt, err := factory.CreateExtension(ctx, extCreateParams, getConfigFn())
-	assert.NoError(t, err)
+	require.NoError(t, err)



Remove new line

bogdandrutu · 2021-03-22T20:15:17Z

service/defaultcomponents/default_processors_test.go

+
+	for _, createFn := range createFns {
+		firstExp, err := createFn(ctx, processorCreateParams, getConfigFn())
+		if err == configerror.ErrDataTypeIsNotSupported {


Same use errors.Is

bogdandrutu · 2021-03-22T20:15:26Z

service/defaultcomponents/default_processors_test.go

+
+		secondExp, err := createFn(ctx, processorCreateParams, getConfigFn())
+		require.NoError(t, err)
+


Same, remove new line

bogdandrutu · 2021-03-22T20:17:07Z

service/defaultcomponents/default_processors_test.go

+func castProcessor(p component.Processor, err error) (component.Processor, error) {
+	return p, err
+}


Not sure I understand this func. Is this necessary? If yes please comment why.

removed, not needed anymore

bogdandrutu · 2021-03-22T20:17:35Z

service/defaultcomponents/default_receivers_test.go

+
+	for _, createFn := range createFns {
+		firstRcvr, err := createFn(ctx, receiverCreateParams, getConfigFn())
+		if err == configerror.ErrDataTypeIsNotSupported {


same comment.

bogdandrutu · 2021-03-22T20:17:41Z

service/defaultcomponents/default_receivers_test.go

+
+		secondRcvr, err := createFn(ctx, receiverCreateParams, getConfigFn())
+		require.NoError(t, err)
+


same comment.

bogdandrutu · 2021-03-22T20:17:49Z

service/defaultcomponents/default_receivers_test.go

+	}
+}
+
+func castReceiver(p component.Receiver, err error) (component.Receiver, error) {


same is this necessary?

removed, not needed anymore.

bogdandrutu · 2021-03-22T20:18:07Z

service/defaultcomponents/default_exporters_test.go

+	}
+}
+
+func castExporter(exp component.Exporter, err error) (component.Exporter, error) {


is this necessary? please comment why.

It was not necessary anymore, perhaps it was needed on the initial version definitely not anymore, removed.

bogdandrutu · 2021-03-22T20:18:51Z

Please consider to update description if necessary.

…etry#2741) Bumps [pygithub](https://github.com/pygithub/pygithub) from 1.58.0 to 1.58.1. - [Release notes](https://github.com/pygithub/pygithub/releases) - [Changelog](https://github.com/PyGithub/PyGithub/blob/v1.58.1/doc/changes.rst) - [Commits](PyGithub/PyGithub@v1.58.0...v1.58.1) --- updated-dependencies: - dependency-name: pygithub dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Add lifecycle tests to all defaultcomponents

42d6c65

pjanotti requested a review from a team March 19, 2021 21:12

tigrannajaryan approved these changes Mar 19, 2021

View reviewed changes

service/defaultcomponents/default_receivers_test.go Outdated Show resolved Hide resolved

pjanotti mentioned this pull request Mar 19, 2021

Allow multiple OTLP to be created #2743

Merged

Remove commented code in test

88603c1

tigrannajaryan reviewed Mar 22, 2021

View reviewed changes

service/defaultcomponents/default_exporters_test.go Outdated Show resolved Hide resolved

Change lifecycle tests per open-telemetry#2757

edb52fe

bogdandrutu reviewed Mar 22, 2021

View reviewed changes

pjanotti added 3 commits March 22, 2021 20:49

Use errors.Is

8ea6abb

Remove extra lines

8a99790

Remove unneeded cast function

b651d96

bogdandrutu approved these changes Mar 22, 2021

View reviewed changes

bogdandrutu merged commit 3660f5c into open-telemetry:main Mar 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lifecycle tests to all default components #2741

Add lifecycle tests to all default components #2741

pjanotti commented Mar 19, 2021

codecov bot commented Mar 19, 2021 •

edited

Loading

tigrannajaryan left a comment

bogdandrutu commented Mar 20, 2021

tigrannajaryan commented Mar 20, 2021

bogdandrutu commented Mar 20, 2021

tigrannajaryan commented Mar 22, 2021

bogdandrutu commented Mar 22, 2021

bogdandrutu Mar 22, 2021

pjanotti Mar 22, 2021

bogdandrutu Mar 22, 2021

pjanotti Mar 22, 2021

bogdandrutu Mar 22, 2021

pjanotti Mar 22, 2021

bogdandrutu Mar 22, 2021

pjanotti Mar 22, 2021

bogdandrutu Mar 22, 2021

pjanotti Mar 22, 2021

bogdandrutu Mar 22, 2021

pjanotti Mar 22, 2021 •

edited

Loading

bogdandrutu Mar 22, 2021

pjanotti Mar 22, 2021

bogdandrutu Mar 22, 2021

pjanotti Mar 22, 2021

bogdandrutu Mar 22, 2021

pjanotti Mar 22, 2021

bogdandrutu Mar 22, 2021

pjanotti Mar 22, 2021

bogdandrutu commented Mar 22, 2021


		secondExp, err := createFn(ctx, expCreateParams, getConfigFn())
		require.NoError(t, err)


		secondExp, err := createFn(ctx, processorCreateParams, getConfigFn())
		require.NoError(t, err)


		secondRcvr, err := createFn(ctx, receiverCreateParams, getConfigFn())
		require.NoError(t, err)

Add lifecycle tests to all default components #2741

Add lifecycle tests to all default components #2741

Conversation

pjanotti commented Mar 19, 2021

Why

What

codecov bot commented Mar 19, 2021 • edited Loading

Codecov Report

tigrannajaryan left a comment

Choose a reason for hiding this comment

bogdandrutu commented Mar 20, 2021

tigrannajaryan commented Mar 20, 2021

bogdandrutu commented Mar 20, 2021

tigrannajaryan commented Mar 22, 2021

bogdandrutu commented Mar 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjanotti Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu commented Mar 22, 2021

codecov bot commented Mar 19, 2021 •

edited

Loading

pjanotti Mar 22, 2021 •

edited

Loading