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

Removed groupbytraceprocessor #1891

Conversation

jpkrohling
Copy link
Member

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

Description:
Removed the groupbytrace processor, as a first step in moving it to the contrib repository.

Link to tracking Issue: None.

Testing: Manual sanity testing.

Documentation: None.

@jpkrohling
Copy link
Member Author

jpkrohling commented Oct 2, 2020

There are a couple of points that I would like to clarify before marking this as ready:

  • This processor exposes metrics. What's the procedure to add that for a processor in contrib? The only place I could think of was in the createTraceProcessor, as I have access to a logger, which I use in case of failure to setup the telemetry.
  • Setting up the telemetry requires having access to the telemetry level, which is part of the package "go.opentelemetry.io/collector/internal/collector/telemetry". As it's an internal package, I can't use this package. What should I use instead?
  • Same question as above, but for "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/common/v1" and "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/trace/v1", used in the tests.

cc @tigrannajaryan, @chris-smith-zocdoc

UPDATE:

  • Metrics: I ended up adding it to the NewFactory() function, swallowing the error.
  • telemetry.Level: I'm not checking this anymore in order to create the metrics. I assume that if people are using this processor, they are interested in its metrics.
  • I migrated the tests to use the official pdata constructs instead of the internal package.

@jpkrohling jpkrohling marked this pull request as draft October 2, 2020 10:30
@codecov
Copy link

codecov bot commented Oct 2, 2020

Codecov Report

Merging #1891 into master will decrease coverage by 0.10%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1891      +/-   ##
==========================================
- Coverage   91.39%   91.28%   -0.11%     
==========================================
  Files         284      278       -6     
  Lines       16740    16356     -384     
==========================================
- Hits        15299    14931     -368     
+ Misses       1009     1000       -9     
+ Partials      432      425       -7     
Impacted Files Coverage Δ
service/defaultcomponents/defaults.go 84.90% <ø> (-0.28%) ⬇️
service/telemetry.go 81.96% <ø> (-0.30%) ⬇️
translator/internaldata/resource_to_oc.go 94.52% <0.00%> (+5.47%) ⬆️

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 9aa0c5e...2dd5903. Read the comment docs.

@tigrannajaryan
Copy link
Member

Let's wait until the processor is added to contrib and then we can remove from core.

@jpkrohling
Copy link
Member Author

Let's wait until the processor is added to contrib and then we can remove from core.

It should actually be the other way around: first it has to be removed from here, then I can update the collector dependency on -contrib to use the latest master. Otherwise, contrib will not start as two processors with the same name exist.

I did try it locally here using the collector builder getting the collector from this PR and adding the processor from the -contrib PR (open-telemetry/opentelemetry-collector-contrib#1179) and can confirm that it works, but we do need to coordinate carefully the two merges. Given that we might have a release this week, I'd merge this after the next release.

@tigrannajaryan
Copy link
Member

Let's wait until the processor is added to contrib and then we can remove from core.

It should actually be the other way around: first it has to be removed from here, then I can update the collector dependency on -contrib to use the latest master. Otherwise, contrib will not start as two processors with the same name exist.

I did try it locally here using the collector builder getting the collector from this PR and adding the processor from the -contrib PR (open-telemetry/opentelemetry-collector-contrib#1179) and can confirm that it works, but we do need to coordinate carefully the two merges. Given that we might have a release this week, I'd merge this after the next release.

OK, let's merge after this week's release.

@tigrannajaryan
Copy link
Member

Please fix the conflict. Core is released, we can merge.

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
@jpkrohling jpkrohling force-pushed the jpkrohling/move-groupbytraceprocessor-to-contrib branch from fed08a6 to 2dd5903 Compare October 9, 2020 08:03
@jpkrohling
Copy link
Member Author

Done!

@jpkrohling
Copy link
Member Author

@pjanotti, could you please review/merge this one?

@jpkrohling
Copy link
Member Author

ping @pjanotti

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

LGTM

@pjanotti pjanotti merged commit ec9d37d into open-telemetry:master Oct 13, 2020
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
* Add semantic convention generator

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

* Update semantic conventions from generator

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

* Use existing internal/tools module

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

* Fix lint issues, more initialisms

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

* Update changelog

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

* semconvgen: Faas->FaaS

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

* Fix a few more key names with replacements

* Update replacements from PR feedback

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

* rename commonInitialisms to capitalizations, move some capitalizations there

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

* Regenerate semantic conventions with updated capitalizations and replacements

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

* Generate semantic conventions from spec v1.3.0

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

* Cleanup semconv generator util a bit

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

* No need to put internal tooling additions in the CHANGELOG

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

* Fix HTTP semconv tests

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>

* Add semconv generation notes to RELEASING.md

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
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.

3 participants