elastic / opentelemetry-lib

Apache License 2.0
0 stars 6 forks source link

Adding k8s remapper for supporting Kibana Inventory "Kubernetes Pod" page #20

Closed gizas closed 2 weeks ago

gizas commented 3 weeks ago

This is the main PR needed for https://github.com/elastic/opentelemetry-dev/issues/180

It adds the remapping functionality for kubeletstats and k8scluster receivers

gizas commented 3 weeks ago

cc @AlexanderWert , @ChrsMark , @tetianakravchenko

This is the relevant PR for the remapping of k8s otel metrics in opentelemetry-lib. Lets sync on how to test it

ChrsMark commented 3 weeks ago

Nice @gizas !

This is the relevant PR for the remapping of k8s otel metrics in opentelemetry-lib. Lets sync on how to test it

I guess we would need to import the opentelemetry-lib in the Elastic Specific processor in order to test this e2e? I assume this Elastic Processor would need to be part of the https://github.com/elastic/opentelemetry-collector-components but I'm not 100% sure if and when this will happen.

@AlexanderWert should we prioritize the Elastic processor so as different teams/contributors can have this as base for the specific implementations that would be on top?

AlexanderWert commented 3 weeks ago

@AlexanderWert should we prioritize the Elastic processor so as different teams/contributors can have this as base for the specific implementations that would be on top?

IIUC, @ishleenk17 is already working on that

ishleenk17 commented 3 weeks ago

@AlexanderWert should we prioritize the Elastic processor so as different teams/contributors can have this as base for the specific implementations that would be on top?

IIUC, @ishleenk17 is already working on that

The code is ready in the private branch and I have pointed @gizas to use that for testing. In the meantime, the PR is being reviewed for merging this code into the components repo.

tetianakravchenko commented 2 weeks ago

@gizas FYI: I've tested this PR - https://github.com/tetianakravchenko/opentelemetry-collector-contrib/commit/808f7f179547910c5897e23f3c9b5346f353ebf3 (changes are based on the mentioned above private branch - https://github.com/tommyers-elastic/opentelemetry-collector-contrib/tree/routing-processor and use the reference to the local opentelemetry-lib)

all works:

Screenshot 2024-06-10 at 17 58 46

(k8s integration assets and the ingest pipeline were installed to make it work)

lahsivjar commented 2 weeks ago

FWIW, to fix the lint we would need to update the notices which could be done by either running make all OR make update-licenses.

gizas commented 2 weeks ago

@lahsivjar np. Do you want me to open a new story? Also I dont have write access to this repo. Can you please merge this PR for me if no other objections?

lahsivjar commented 2 weeks ago

@gizas Yes, let's open a tracking issue for the test package.

@ishleenk17 Could you take a 👀 and merge if all good?

ishleenk17 commented 2 weeks ago

@gizas : Changes look inline with the hostmetrics code. Have we tested these changes, with the processor?

gizas commented 2 weeks ago

@ishleenk17 not with the elasticinframetricsprocessor (as we will need to make changes here is not it?)

But we repeated the tests as mentioned here

Do we need to open a PR there as well?

@lahsivjar FYI: https://github.com/elastic/opentelemetry-lib/issues/22

ishleenk17 commented 2 weeks ago

(as we will need to make changes here is not it?)

Yes changes would be needed in this code as well.

But we repeated the tests as mentioned here

Ok, if we have done these, mostly should be ok by using the latest elasticinframetrics procesor as well.

Do we need to open a PR there as well?

Yes, if changes are needed in processor, you should open a PR for that also.

For now, from the library perspective the changes are good to go!

tetianakravchenko commented 1 week ago

Please add the invocation change required in the Elasticinframetricscode as well.

@gizas FYI: https://github.com/elastic/opentelemetry-collector-components/pull/14