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.

Sign up for GitHub

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

Jump to bottom

Add prometheus as metric exporter #1509

Merged
merged 3 commits into from
Sep 22, 2023
Merged

Add prometheus as metric exporter #1509

merged 3 commits into from
Sep 22, 2023

Conversation

LionelJouin
Copy link
Member

Description

An init method supporting prometheus for the metric exporter has been added for the opentelemetry pkg. NSMgr or any metric collector could now export the metrics via prometheus.
span trace and metrics can now be individually disabled by passing nil value to the optl init function.

Issue link

/

How Has This Been Tested?

  • Added unit testing to cover
  • Tested manually
  • Tested by integration testing
  • Have not tested

Types of changes

  • Bug fix
  • New functionality
  • Documentation
  • Refactoring
  • CI

@LionelJouin
Add prometheus as metric exporter
ca604b9 
An init method supporting prometheus for the metric exporter has been added for the
opentelemetry pkg. NSMgr or any metric collector could now export the
metrics via prometheus.
span trace and metrics can now be individually disabled by passing nil
value to the optl init function.

Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>
@codecov
Copy link

codecov bot commented Sep 11, 2023 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (main@f96fdf6). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1509   +/-   ##
=======================================
  Coverage        ?   70.15%           
=======================================
  Files           ?      248           
  Lines           ?    11203           
  Branches        ?        0           
=======================================
  Hits            ?     7860           
  Misses          ?     2842           
  Partials        ?      501

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LionelJouin LionelJouin mentioned this pull request Sep 11, 2023
Open
denis-tingaikin
denis-tingaikin reviewed Sep 11, 2023
View reviewed changes
pkg/tools/opentelemetry/exporter.go Outdated
// InitMetricExporter - returns an instance of OpenTelemetry Metric Exporter.
func InitMetricExporter(ctx context.Context, exporterURL string) *sdkmetric.Exporter {
// InitOPTLMetricExporter - returns an instance of OpenTelemetry Metric Exporter.
func InitOPTLMetricExporter(ctx context.Context, exporterURL string) sdkmetric.Reader {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the change because it breaks compatibility with cmd apps.

Could you explain how is critical to change signature?

Copy link
Member Author

Choose a reason for hiding this comment

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

The open telemetry SDK returns a metric.Reader for Prometheus for the prometheus exporter.
https://github.com/open-telemetry/opentelemetry-go/blob/v1.16.0/exporters/prometheus/exporter.go#L53
From what I understood with the open telemetry SDK, metric.Reader is more generic way to be used as way of collecting data than sdkmetric.Exporter

Copy link
Member

Choose a reason for hiding this comment

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

OK, got it.
Do you need the change in release v1.11.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

@szvincze ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right now I don't think it is critical to have it in 1.11.0.

Copy link
Member

Choose a reason for hiding this comment

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

@LionelJouin , @szvincze OK, let's target this for release 1.12.0 because this change will require updating all cmd apps due to the changing contract of function InitMetricExporter -> InitOPTLMetricExporter

Copy link
Contributor

Choose a reason for hiding this comment

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

@LionelJouin , @szvincze OK, let's target this for release 1.12.0 because this change will require updating all cmd apps due to the changing contract of function InitMetricExporter -> InitOPTLMetricExporter

@denis-tingaikin: We had some internal discussions about this topic. It seems this change would save quite significant effort for us if we could get it in 1.11.0. Is it possible to consider it in 1.11.0? Is there other alternative to get it before 1.12.0?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but we'll need a help with updating cmd-repos.

Also, we supposed to be ready for delaying in release if something went wrong.

@edwarnicke Don't you mind if we consider this PR in this release (v.1.11.0)?

Copy link
Member

Choose a reason for hiding this comment

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

@denis-tingaikin Go for it

@denis-tingaikin denis-tingaikin self-assigned this Sep 11, 2023
NikitaSkrynnik
NikitaSkrynnik reviewed Sep 15, 2023
View reviewed changes
pkg/tools/opentelemetry/exporter.go Outdated
return &exporter
return sdkmetric.NewPeriodicReader(
exporter,
sdkmetric.WithInterval(10*time.Second),
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it better to make it configurable?

Copy link
Member Author

Choose a reason for hiding this comment

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

do you mean the 10 seconds?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

@LionelJouin
Update open telemetry dependencies
1eac726 
Update to v1.19.0-rc.1 and v0.42.0 which are fixing bugs related to
Prometheus exporter.
Remove Jaeger exporter since it is deprecated

Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>
@LionelJouin
Add export interval parameter for the optl exporter
e965965 
Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>
@denis-tingaikin
Copy link
Member

denis-tingaikin commented Sep 21, 2023 •
edited
Loading

@LionelJouin try run go mod tidy to make it green.

@LionelJouin
Copy link
Member Author

I think the CI needs an update, I get this error with go 1.20.5 (1.20.8 and 1.21.1 are working fine):

github.com/networkservicemesh/sdk/pkg/tools/tracing imports
        go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc tested by
        go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc.test imports
        google.golang.org/grpc/interop imports
        golang.org/x/oauth2/google imports
        cloud.google.com/go/compute/metadata: ambiguous import: found package cloud.google.com/go/compute/metadata in multiple modules:
        cloud.google.com/go v0.34.0 (/home/lionelj/go/pkg/mod/cloud.google.com/go@v0.34.0/compute/metadata)
        cloud.google.com/go/compute/metadata v0.2.3 (/home/lionelj/go/pkg/mod/cloud.google.com/go/compute/metadata@v0.2.3)

@denis-tingaikin
Copy link
Member

@LionelJouin I think we may start with v1.20.8 https://github.com/networkservicemesh/.github/blob/main/.github/workflows/build-and-test.yaml#L22
Could you open PR?

@LionelJouin
Copy link
Member Author

Sure: networkservicemesh/.github#45

denis-tingaikin
denis-tingaikin approved these changes Sep 21, 2023
View reviewed changes
Copy link
Member

@denis-tingaikin denis-tingaikin left a comment

Choose a reason for hiding this comment

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

LGTM

@glazychev-art , @NikitaSkrynnik Could you also have a look?

@denis-tingaikin denis-tingaikin merged commit 97bacd7 into networkservicemesh:main Sep 22, 2023
nsmbot pushed a commit to networkservicemesh/cmd-csi-driver that referenced this pull request Sep 22, 2023
Update go.mod and go.sum to latest version from networkservicemesh/sd…
ec52b70 
…k@main

PR link: networkservicemesh/sdk#1509

Commit: 97bacd7
Author: Lionel Jouin
Date: 2023-09-22 12:06:10 +0200
Message:
  - Add prometheus as metric exporter (#1509)
* Add prometheus as metric exporter

An init method supporting prometheus for the metric exporter has been added for the
opentelemetry pkg. NSMgr or any metric collector could now export the
metrics via prometheus.
span trace and metrics can now be individually disabled by passing nil
value to the optl init function.

Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>

* Update open telemetry dependencies

Update to v1.19.0-rc.1 and v0.42.0 which are fixing bugs related to
Prometheus exporter.
Remove Jaeger exporter since it is deprecated

Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>

* Add export interval parameter for the optl exporter

Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>

---------

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-cluster-info-k8s that referenced this pull request Sep 22, 2023
Update go.mod and go.sum to latest version from networkservicemesh/sd…
3e0dbb3 
…k@main

PR link: networkservicemesh/sdk#1509

Commit: 97bacd7
Author: Lionel Jouin
Date: 2023-09-22 12:06:10 +0200
Message:
  - Add prometheus as metric exporter (#1509)
* Add prometheus as metric exporter

An init method supporting prometheus for the metric exporter has been added for the
opentelemetry pkg. NSMgr or any metric collector could now export the
metrics via prometheus.
span trace and metrics can now be individually disabled by passing nil
value to the optl init function.

Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>

* Update open telemetry dependencies

Update to v1.19.0-rc.1 and v0.42.0 which are fixing bugs related to
Prometheus exporter.
Remove Jaeger exporter since it is deprecated

Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>

* Add export interval parameter for the optl exporter

Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>

---------

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/sdk-kernel that referenced this pull request Sep 22, 2023
Update go.mod and go.sum to latest version from networkservicemesh/sd…
a8f90ae 
…k@main

PR link: networkservicemesh/sdk#1509

Commit: 97bacd7
Author: Lionel Jouin
Date: 2023-09-22 12:06:10 +0200
Message:
  - Add prometheus as metric exporter (#1509)
* Add prometheus as metric exporter

An init method supporting prometheus for the metric exporter has been added for the
opentelemetry pkg. NSMgr or any metric collector could now export the
metrics via prometheus.
span trace and metrics can now be individually disabled by passing nil
value to the optl init function.

Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>

* Update open telemetry dependencies

Update to v1.19.0-rc.1 and v0.42.0 which are fixing bugs related to
Prometheus exporter.
Remove Jaeger exporter since it is deprecated

Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>

* Add export interval parameter for the optl exporter

Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>

---------

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr-proxy that referenced this pull request Sep 22, 2023
Update go.mod and go.sum to latest version from networkservicemesh/sd…
c7856ec 
…k@main

PR link: networkservicemesh/sdk#1509

Commit: 97bacd7
Author: Lionel Jouin
Date: 2023-09-22 12:06:10 +0200
Message:
  - Add prometheus as metric exporter (#1509)
* Add prometheus as metric exporter

An init method supporting prometheus for the metric exporter has been added for the
opentelemetry pkg. NSMgr or any metric collector could now export the
metrics via prometheus.
span trace and metrics can now be individually disabled by passing nil
value to the optl init function.

Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>

* Update open telemetry dependencies

Update to v1.19.0-rc.1 and v0.42.0 which are fixing bugs related to
Prometheus exporter.
Remove Jaeger exporter since it is deprecated

Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>

* Add export interval parameter for the optl exporter

Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>

---------

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
@nsmbot nsmbot mentioned this pull request Sep 22, 2023
Merged
This was referenced Sep 25, 2023
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Closed
Closed
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szvincze szvincze szvincze left review comments

@edwarnicke edwarnicke edwarnicke left review comments

@NikitaSkrynnik NikitaSkrynnik NikitaSkrynnik approved these changes

@denis-tingaikin denis-tingaikin denis-tingaikin approved these changes

@bellycat77 bellycat77 bellycat77 approved these changes

@dualBreath dualBreath Awaiting requested review from dualBreath

@glazychev-art glazychev-art Awaiting requested review from glazychev-art

@d-uzlov d-uzlov Awaiting requested review from d-uzlov

Assignees

@NikitaSkrynnik NikitaSkrynnik

Labels
None yet
Projects
Release v1.11.0
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@LionelJouin @denis-tingaikin @edwarnicke @bellycat77 @szvincze @NikitaSkrynnik