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

Migrate Resource Processor to internal data model #1315

Merged

Conversation

dmitryax
Copy link
Member

@dmitryax dmitryax commented Jul 10, 2020

Description:
This PR migrates resource processor to internal data model.

Existing processor configuration is relevant only to OpenCensus format, so the configuration schema has to be changed. Taking this opportunity, this commit adds existing logic for attributes manipulation from attributes processor to resource processor.
New config uses "attributes" field which represents actions that can be made on resource attributes. Supported actions: INSERT, UPDATE, UPSERT, DELETE, HASH, EXTRACT.
In order to migrate existing resource processor config:

  1. Move key/values from "labels" field to "attributes" with action="upsert".
  2. Add value from "type" field to "attributes" with key="opencensus.resourcetype" and action="upsert".

Link to tracking Issue:
Resolves: #1307

Testing:
Added unit tests

@codecov
Copy link

codecov bot commented Jul 10, 2020

Codecov Report

Merging #1315 into master will increase coverage by 0.00%.
The diff coverage is 98.41%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1315   +/-   ##
=======================================
  Coverage   89.61%   89.61%           
=======================================
  Files         215      215           
  Lines       15181    15185    +4     
=======================================
+ Hits        13604    13608    +4     
- Misses       1149     1150    +1     
+ Partials      428      427    -1     
Impacted Files Coverage Δ
internal/processor/attraction/attraction.go 100.00% <ø> (ø)
processor/resourceprocessor/resource_processor.go 75.00% <96.42%> (-11.89%) ⬇️
processor/resourceprocessor/factory.go 100.00% <100.00%> (ø)
translator/internaldata/resource_to_oc.go 88.37% <0.00%> (+2.32%) ⬆️

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 1c85165...b489311. Read the comment docs.

@dmitryax dmitryax force-pushed the migrate-resource-processor branch 2 times, most recently from bc59aed to 4963e9e Compare July 10, 2020 04:42
resource/2:
type: "host"
Copy link
Member

@nilebox nilebox Jul 10, 2020

Choose a reason for hiding this comment

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

Can you preserve the resource type for backward compatibility, please?
I believe it will be an attribute

"opencensus.resourcetype": "host"

based on

OCAttributeResourceType = "opencensus.resourcetype"

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was also thinking about it, but since we are in beta state and going through some breaking changes between releases anyway, decided to drop it right away.
But probably you right, it might be too aggressive and we can mark it as deprecated for now ...

Copy link
Member Author

Choose a reason for hiding this comment

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

done

TypeVal: "resource",
NameVal: "resource",
},
ResourceType: "host",
Copy link
Member

Choose a reason for hiding this comment

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

Same here: please replace this with "opencensus.type": "host" attribute rather than deleting it.

Copy link
Member Author

Choose a reason for hiding this comment

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

added the backward compatibility

@dmitryax dmitryax force-pushed the migrate-resource-processor branch from 4963e9e to a2bcbc2 Compare July 10, 2020 07:56
@dmitryax dmitryax changed the title [BREAKING CHANGE] Migrate Resource Processor to internal data model Migrate Resource Processor to internal data model Jul 10, 2020
@dmitryax dmitryax force-pushed the migrate-resource-processor branch 5 times, most recently from fdb3761 to 9b40e32 Compare July 10, 2020 17:17
"cloud.zone": "zone-1",
"k8s.cluster.name": "k8s-cluster",
"host.name": "k8s-node",
attributes:
Copy link
Member

Choose a reason for hiding this comment

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

As discussed offline I think we can borrow the attribute processor actions implementation and config for this.
BTW, if need specifically a rename we can add it as an action so that we don't have to use upsert+delete pair all the time.

Copy link
Member

Choose a reason for hiding this comment

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

Code was extracted and easy to use.

@dmitryax dmitryax force-pushed the migrate-resource-processor branch 7 times, most recently from 20c4538 to f529c41 Compare July 12, 2020 00:31
resource: resource,
capabilities: component.ProcessorCapabilities{MutatesConsumedData: !isEmptyResource(resource)},
attrProc: attrProc,
capabilities: component.ProcessorCapabilities{MutatesConsumedData: attrProc.HasActions()},
Copy link
Member

Choose a reason for hiding this comment

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

File an issue to implement same behavior as attributes span processor which fails on empty config. Or you can change this to have the same behavior.

@dmitryax dmitryax force-pushed the migrate-resource-processor branch from f529c41 to c606a7e Compare July 12, 2020 01:57
This commit migrates resource processor to internal data model. Existing processor configuration is relevant only to OpenCensus format, so the configuration schema has to be changed. Taking this opportunity, this commit adds existing logic for attributes manipulation from attributes processor to resource processor. 
New config uses "attributes" field which represents actions that can be made on resource attributes. Supported actions: INSERT, UPDATE, UPSERT, DELETE, HASH, EXTRACT.
In order to migrate existing resource processor config:
1. Move key/values from "labels" field to "attributes" with action="upsert".
2. Add value from "type" field to "attributes" with key="opencensus.resourcetype"  and action="upsert".
@dmitryax dmitryax force-pushed the migrate-resource-processor branch from c606a7e to b489311 Compare July 12, 2020 02:08
@bogdandrutu bogdandrutu merged commit 3711c01 into open-telemetry:master Jul 12, 2020
wyTrivail pushed a commit to mxiamxia/opentelemetry-collector that referenced this pull request Jul 13, 2020
This commit migrates resource processor to internal data model. Existing processor configuration is relevant only to OpenCensus format, so the configuration schema has to be changed. Taking this opportunity, this commit adds existing logic for attributes manipulation from attributes processor to resource processor. 
New config uses "attributes" field which represents actions that can be made on resource attributes. Supported actions: INSERT, UPDATE, UPSERT, DELETE, HASH, EXTRACT.
In order to migrate existing resource processor config:
1. Move key/values from "labels" field to "attributes" with action="upsert".
2. Add value from "type" field to "attributes" with key="opencensus.resourcetype"  and action="upsert".
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
…etry#1315)

Bumps [github.com/golangci/golangci-lint](https://github.com/golangci/golangci-lint) from 1.32.1 to 1.32.2.
- [Release notes](https://github.com/golangci/golangci-lint/releases)
- [Changelog](https://github.com/golangci/golangci-lint/blob/master/CHANGELOG.md)
- [Commits](golangci/golangci-lint@v1.32.1...v1.32.2)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
swiatekm pushed a commit to swiatekm/opentelemetry-collector that referenced this pull request Oct 9, 2024
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.

Migrate Resource Processor to the new OTLP-based internal data model
4 participants