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

feat: Oltp metrics exporter #3021

Merged
merged 12 commits into from
Apr 24, 2024
Merged

feat: Oltp metrics exporter #3021

merged 12 commits into from
Apr 24, 2024
+404 −46

Conversation

markphelps
Copy link
Collaborator

Fixes: #2997

@markphelps
chore: mod tidy
6976353
@markphelps
Merge branch 'main' of https://github.com/flipt-io/flipt
1292101 
* 'main' of https://github.com/flipt-io/flipt:
  chore(deps): bump golang.org/x/net in /examples/openfeature (#3003)
  chore(deps): bump github.com/docker/docker in /build (#2999)
  chore(deps): bump github.com/go-git/go-git/v5 from 5.11.0 to 5.12.0 (#3012)
  chore(deps): bump go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc (#3009)
  chore(deps): bump github.com/go-sql-driver/mysql from 1.8.0 to 1.8.1 (#3011)
  chore(deps): bump golang.org/x/crypto from 0.21.0 to 0.22.0 (#3010)
@markphelps
Merge branch 'main' of https://github.com/flipt-io/flipt
ccb608b 
* 'main' of https://github.com/flipt-io/flipt:
  chore(deps): bump golang.org/x/net from 0.22.0 to 0.23.0 in /rpc/flipt (#3004)
@markphelps
chore: mod tidy
d17db5d
@markphelps markphelps added the needs docs Requires documentation updates label Apr 24, 2024
@markphelps
Merge branch 'main' of https://github.com/flipt-io/flipt
6a5e10d 
* 'main' of https://github.com/flipt-io/flipt:
  chore(deps): bump golang.org/x/net from 0.22.0 to 0.23.0 in /sdk/go (#3006)
  chore(deps): bump golang.org/x/net from 0.22.0 to 0.23.0 in /core (#3005)
  chore: update changelog for v1.40.2 (#3020)
  refactor(storage): move from lib/pq to pgx (#2996)
  fix(cmd/grpc): skip eval data service when evaluation API auth excluded (#3018)
@markphelps
feat: impl otel metrics exporter and config
66f1aa3 
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
@markphelps
chore: fix config tests; make noop global meter
47f9ddd 
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
@markphelps
chore: add schema
Loading
Loading status checks…
aa2d99a 
Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
@markphelps markphelps marked this pull request as ready for review April 24, 2024 14:50
@markphelps markphelps requested a review from a team as a code owner April 24, 2024 14:50
@markphelps
chore: respond to PR feedback
Loading
Loading status checks…
043fc7c
@markphelps markphelps requested review from GeorgeMac and erka April 24, 2024 17:33
GeorgeMac
GeorgeMac approved these changes Apr 24, 2024
View reviewed changes
Copy link
Contributor

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

Nice

@erka
Copy link
Contributor

erka commented Apr 24, 2024 •
edited
Loading

Something is wrong here. The code looks good but there is not data at /metrics. With 1.40.1 I have these records

flipt_evaluations_results_total{flag="flag1",match="false",namespace="default",otel_scope_name="github.com/flipt-io/flipt",otel_scope_version="",reason="UNKNOWN_EVALUATION_REASON",segment="",type="variant",value=""} 17
flipt_evaluations_results_total{flag="flag1",match="true",namespace="default",otel_scope_name="github.com/flipt-io/flipt",otel_scope_version="",reason="MATCH_EVALUATION_REASON",segment="demo",type="variant",value="btn"} 12

@markphelps
Copy link
Collaborator Author

Something is wrong here. The code looks good but there is not data at /metrics. With 1.40.1 I have there records

flipt_evaluations_results_total{flag="flag1",match="false",namespace="default",otel_scope_name="github.com/flipt-io/flipt",otel_scope_version="",reason="UNKNOWN_EVALUATION_REASON",segment="",type="variant",value=""} 17
flipt_evaluations_results_total{flag="flag1",match="true",namespace="default",otel_scope_name="github.com/flipt-io/flipt",otel_scope_version="",reason="MATCH_EVALUATION_REASON",segment="demo",type="variant",value="btn"} 12

Hmm I tested it locally using our Docker Compose metrics example and I got data in prometheus

CleanShot 2024-04-24 at 14 30 09

@markphelps
Copy link
Collaborator Author

But you're right I dont see flipt_evaluations_ metrics

@markphelps
fix: dont override set meter provider
Loading
Loading status checks…
4bd8d49
@markphelps
Copy link
Collaborator Author

CleanShot 2024-04-24 at 14 38 59

@erka I think i just fixed it with my last commit

@markphelps
chore: add basic IT for metrics
Loading
Loading status checks…
c107156
@markphelps
Copy link
Collaborator Author

I added a basic IT to catch this going forward

@markphelps markphelps mentioned this pull request Apr 24, 2024
Merged
@markphelps markphelps requested a review from GeorgeMac April 24, 2024 19:42
@markphelps markphelps added the automerge Used by Kodiak bot to automerge PRs label Apr 24, 2024
@kodiakhq kodiakhq bot merged commit 2ca5dfb into main Apr 24, 2024
29 checks passed
@kodiakhq kodiakhq bot deleted the oltp-metrics branch April 24, 2024 19:49
@markphelps
Copy link
Collaborator Author

Oops. did not know this would automerge without requiring re-review 😲

@markphelps
Copy link
Collaborator Author

If theres any more feedback on this PR i can fix in a another branch before release, so lmk!

@erka
Copy link
Contributor

erka commented Apr 24, 2024

@markphelps it looks good. One thing that could be wrong that http should have insecure option. Please look at this

@markphelps
Copy link
Collaborator Author

@markphelps it looks good. One thing that could be wrong that http should have insecure option. Please look at this

Do you mean so that someone could configure it to be secure? Im curious as to why someone would want to do that? reading the code you linked it looks like it defaults to set insecure if protocol == http

markphelps added a commit that referenced this pull request Apr 24, 2024
@markphelps
Merge branch 'main' of https://github.com/flipt-io/flipt
40610b3 
* 'main' of https://github.com/flipt-io/flipt:
  feat: Oltp metrics exporter (#3021)
@erka
Copy link
Contributor

erka commented Apr 25, 2024 •
edited
Loading

Do you mean so that someone could configure it to be secure? Im curious as to why someone would want to do that? reading the code you linked it looks like it defaults to set insecure if protocol == http

If you check the otlp client it uses https and only switches to http with insecure option. @markphelps

@markphelps
Copy link
Collaborator Author

Ah I see! Yes will create a new pr for http support both here and for OTLP tracing

@erka
Copy link
Contributor

erka commented Apr 25, 2024

@markphelps there is one more thing about prometheus

flipt/internal/cmd/grpc.go

Lines 434 to 435 in 2ca5dfb

grpc_prometheus.EnableHandlingTimeHistogram()
grpc_prometheus.Register(server.Server)

There could be some activity with it even the configuration has 'metrics' disabled or otlp is set. I personally don't see the issue here but the documentation may need some adjustments.

@markphelps markphelps mentioned this pull request Apr 25, 2024
Merged
@markphelps markphelps removed the needs docs Requires documentation updates label May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erka erka

@GeorgeMac GeorgeMac

Assignees
No one assigned
Labels
automerge Used by Kodiak bot to automerge PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow multiple metrics exporter (Prometheus, OpenTelemetry)
3 participants
@markphelps @erka @GeorgeMac