feat: Add open telemetry metrics and traces #3404

nasdf · 2025-01-23T22:55:21Z

Relevant issue(s)

Resolves #293
Resolves #74

Description

This PR adds OpenTelemetry metrics and tracing. Telemetry is only enabled if the telemetry build flag is set.

Our default telemetry configuration uses the http exporter, but we can expand it to grpc and console if needed.

The exporters are configured through the standardized OpenTelemetry environment variables found here:

https://pkg.go.dev/go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp
https://pkg.go.dev/go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Manually testing with Jaeger

docker run --rm --name jaeger \
  -p 16686:16686 \
  -p 4317:4317 \
  -p 4318:4318 \
  -p 5778:5778 \
  -p 9411:9411 \
  jaegertracing/jaeger:2.2.0

DEFRA_KEYRING_SECRET=secret \
 OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4318 \
 go run -tags telemetry ./cmd/defradb start

Trace dashboard

Detailed trace example

Nested trace example

Specify the platform(s) on which this was tested:

MacOS

AndrewSisley · 2025-01-23T23:02:25Z

internal/db/db.go

@@ -39,7 +39,8 @@ import (
 )

 var (
-	log = corelog.NewLogger("db")
+	log    = corelog.NewLogger("db")
+	tracer = otel.Tracer("github.com/sourcenetwork/defradb/internal/db")


thought: I think I agree with John's suggestion of extracting this out to a thin package so we can avoid scattering references throughout the code base to the same 3rd party package.

If we ever swap it out we'll probably still need to change the func signatures (I wouldn't put much thought into designing them), but at least everything will be in the same place, and it makes adding/standardising any middleware much easier.

Update: the metrics were all moved to the internal metric package and put behind a build flag telemetry.

AndrewSisley · 2025-01-23T23:05:02Z

internal/db/db.go

@@ -212,6 +213,9 @@ func (db *DB) AddPolicy(
 	ctx context.Context,
 	policy string,
 ) (client.AddPolicyResult, error) {
+	ctx, span := tracer.Start(ctx, "AddPolicy")


suggestion: I do not know about performance/C-GO, but we can probably avoid having to manually specify "AddPolicy" by using the runtime package and stuff like runtime.CallersFrames instead.

I can look into that.

Update: I was able to get this working. Thanks for the suggestion!

Maybe I am missing something, but I still see the manual specification.

The preview above is outdated. Should be fixed in the actual code.

codecov · 2025-01-23T23:14:36Z

Codecov Report

Attention: Patch coverage is 84.58150% with 35 lines in your changes missing coverage. Please review.

Project coverage is 78.34%. Comparing base (d6b003b) to head (4c4f59d).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
cli/start.go	35.71%	9 Missing ⚠️
internal/telemetry/otel.go	83.33%	6 Missing and 3 partials ⚠️
internal/telemetry/telemetry.go	40.00%	6 Missing and 3 partials ⚠️
internal/db/collection.go	85.71%	3 Missing ⚠️
internal/db/collection_delete.go	0.00%	3 Missing ⚠️
internal/telemetry/noop.go	71.43%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3404      +/-   ##
===========================================
+ Coverage    78.27%   78.34%   +0.07%     
===========================================
  Files          393      395       +2     
  Lines        36113    36286     +173     
===========================================
+ Hits         28266    28426     +160     
- Misses        6187     6194       +7     
- Partials      1660     1666       +6

Flag	Coverage Δ
all-tests	`78.34% <84.58%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
cli/config.go	`80.00% <ø> (ø)`
internal/db/collection_get.go	`80.65% <100.00%> (-4.10%)`	⬇️
internal/db/collection_index.go	`87.82% <100.00%> (+0.28%)`	⬆️
internal/db/collection_update.go	`74.80% <100.00%> (+0.61%)`	⬆️
internal/db/db.go	`67.98% <100.00%> (+1.18%)`	⬆️
internal/db/p2p_replicator.go	`62.33% <100.00%> (+0.60%)`	⬆️
internal/db/p2p_schema_root.go	`80.92% <100.00%> (-0.78%)`	⬇️
internal/db/request.go	`87.88% <100.00%> (+5.02%)`	⬆️
internal/db/schema.go	`84.81% <100.00%> (ø)`
internal/db/store.go	`73.89% <100.00%> (+8.70%)`	⬆️
... and 8 more

... and 15 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6b003b...4c4f59d. Read the comment docs.

AndrewSisley

praise: This looks really good considering my natural aversion to it, good job at minimising it's impact on readability.

I have not really reviewed where you have added the tracer (i.e. all the tracer.Start calls), as I would prefer to not have any by default and instead add them in when we really feel the need to (like debug logs) - I will let someone else review those.

todo: Please add a job to the CI test matrix so that we have one job that executes the tests with the tracer enabled.

AndrewSisley · 2025-01-29T22:59:51Z

internal/request/graphql/parser.go

@@ -46,7 +49,10 @@ func NewParser() (*parser, error) {
 	return p, nil
 }

-func (p *parser) BuildRequestAST(request string) (*ast.Document, error) {
+func (p *parser) BuildRequestAST(ctx context.Context, request string) (*ast.Document, error) {
+	_, span := tracer.Start(ctx)


question: Why are you not assigning the return value to ctx here? It looks like a bug-in-waiting. Did the compiler/linter complain?

Yes, it was a linter warning because the context is only used for creating the span. The reason for adding the context to the function signature is that it allows the creation of nested spans by using context values.

Ah okay, thanks for the answer.

I think I have a slight preference to assigning it with a //nolint:foo comment as that would be less risky in case we later use ctx in this function, but it is a bit ugly so please don't feel any pressure to change it if you aren't certain in your agreement :)

I do have a slightly stronger preference to leave as it was and not do the linter suppression. Sorry Keenan

I ended up reverting the change since more of us preferred the original.

shahzadlone · 2025-01-30T05:01:31Z

cli/start.go

+			if !cfg.GetBool("no-telemetry") {
+				err := metric.ConfigureTelemetry(cmd.Context())
+				if err != nil {
+					log.ErrorE("failed to configure telemetry", err)


thought: Perhaps define in cli/errors.go

(thought because its a log).

internal/metric/metric_test.go

internal/metric/noop.go

tests/integration/utils.go

shahzadlone

Good stuff, just left some comments.

Please do add the CI test with the tracer enabled (doesn't have to be a matrix) just one workflow is good. You can do something like the test-view or test-encryption job in .github/workflows/test-coverage.yml.

fredcarle · 2025-01-30T16:01:50Z

docs/website/references/cli/defradb_start.md

@@ -21,6 +21,7 @@ defradb start [flags]
      --max-txn-retries int           Specify the maximum number of retries per transaction (default 5)
      --no-encryption                 Skip generating an encryption key. Encryption at rest will be disabled. WARNING: This cannot be undone.
      --no-p2p                        Disable the peer-to-peer network synchronization system
+      --no-telemetry telemetry        Disables telemetry reporting. Telemetry is only enabled in builds that use the telemetry flag.


question: Is telemetry next to --no-telemetry supposed to be there?

good catch! should be fixed now

shahzadlone · 2025-01-30T19:56:16Z

.github/workflows/test-coverage.yml

+      - name: Test coverage & save coverage report in an artifact
+        uses: ./.github/composites/test-coverage-with-artifact
+        with:
+          coverage-artifact-name: "coverage_encryption"


issue: This typo is very problematic as it will fail all reports due to duplicate artifact name (or overwrite which was the old behavior). Please fix the name to something relevant.

nice catch! should be fixed now

.github/workflows/test-coverage.yml

shahzadlone · 2025-02-04T03:23:31Z

internal/metric/otel_test.go

+
+//go:build telemetry
+
+package metric


nitpick: Feel free to rename the package to telemetry at this point.

shahzadlone · 2025-02-04T03:38:22Z

internal/request/graphql/parser.go

@@ -50,7 +50,7 @@ func NewParser() (*parser, error) {
 }

 func (p *parser) BuildRequestAST(ctx context.Context, request string) (*ast.Document, error) {
-	_, span := tracer.Start(ctx)
+	ctx, span := tracer.Start(ctx) //nolint:ineffassign,staticcheck


todo: Why do you need to do this. 1) ctx is not used anywhere and 2) it is ineffectual assignment.

I like what you had before.

Same for all other instances

He change it because Andy asked him to do so I think. I'm also not a fan of this change.

Didn't seem like a strong preference by him, I commented on the original thread now.

AndrewSisley

LGTM, thanks Keenan!

nasdf added 2 commits January 23, 2025 14:51

remove internal metrics package

815e8c3

add open telemetry setup and basic traces

edfc9c7

nasdf self-assigned this Jan 23, 2025

AndrewSisley reviewed Jan 23, 2025

View reviewed changes

nasdf added 6 commits January 23, 2025 15:48

move trace logic to metric package

5eef3d7

move all otel code to metric package

ccc8f17

add traces to request parser

a76f789

add telemetry config and cli flag

57934a3

fix metric call info

abf21ee

improve otel comments

7d9a96e

nasdf changed the title ~~feat: OpenTelemetry [DO NOT REVIEW]~~ feat: Add open telemetry metrics and traces Jan 29, 2025

nasdf marked this pull request as ready for review January 29, 2025 21:36

nasdf requested review from a team and AndrewSisley January 29, 2025 21:37

AndrewSisley requested changes Jan 29, 2025

View reviewed changes

shahzadlone reviewed Jan 30, 2025

View reviewed changes

shahzadlone approved these changes Jan 30, 2025

View reviewed changes

fredcarle reviewed Jan 30, 2025

View reviewed changes

nasdf added 2 commits January 30, 2025 10:15

PR fixes

7112dbe

add telemetry test workflow

b1b5c53

shahzadlone reviewed Jan 30, 2025

View reviewed changes

nasdf added 2 commits January 30, 2025 13:32

fix workflow coverage artifact name

6863ff5

Merge branch 'develop' into nasdf/feat/open-telemetry

21a4ba1

shahzadlone reviewed Jan 30, 2025

View reviewed changes

.github/workflows/test-coverage.yml Outdated Show resolved Hide resolved

go mod tidy

237f8a5

shahzadlone reviewed Jan 30, 2025

View reviewed changes

.github/workflows/test-coverage.yml Outdated Show resolved Hide resolved

nasdf added 2 commits January 31, 2025 11:23

fix GO_FLAGS in test workflow

6b624f4

pr fixes

64a98d8

shahzadlone reviewed Feb 4, 2025

View reviewed changes

nasdf added 6 commits February 6, 2025 08:56

rename metric package to telemetry

1cfe3ea

remove unused context nolint

1c57f99

add telemetry readme section

76b0958

Merge branch 'develop' into nasdf/feat/open-telemetry

1693abd

fix telemetry test

48e0b1e

fix package name

4c4f59d

AndrewSisley approved these changes Feb 7, 2025

View reviewed changes

nasdf merged commit 34eab3f into sourcenetwork:develop Feb 7, 2025
43 of 44 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add open telemetry metrics and traces #3404

feat: Add open telemetry metrics and traces #3404

nasdf commented Jan 23, 2025 •

edited

Loading

AndrewSisley Jan 23, 2025

nasdf Jan 29, 2025

AndrewSisley Jan 23, 2025

nasdf Jan 23, 2025

nasdf Jan 29, 2025

shahzadlone Feb 6, 2025

nasdf Feb 6, 2025

codecov bot commented Jan 23, 2025 •

edited

Loading

AndrewSisley left a comment

AndrewSisley Jan 29, 2025

nasdf Jan 29, 2025

AndrewSisley Jan 29, 2025

shahzadlone Feb 4, 2025 •

edited

Loading

nasdf Feb 6, 2025

shahzadlone Jan 30, 2025

shahzadlone left a comment

fredcarle Jan 30, 2025

nasdf Jan 30, 2025

shahzadlone Jan 30, 2025

nasdf Jan 30, 2025

shahzadlone Feb 4, 2025

nasdf Feb 6, 2025

shahzadlone Feb 4, 2025

fredcarle Feb 4, 2025

shahzadlone Feb 4, 2025

AndrewSisley left a comment

feat: Add open telemetry metrics and traces #3404

feat: Add open telemetry metrics and traces #3404

Conversation

nasdf commented Jan 23, 2025 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 23, 2025 • edited Loading

Codecov Report

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

nasdf commented Jan 23, 2025 •

edited

Loading

codecov bot commented Jan 23, 2025 •

edited

Loading

shahzadlone Feb 4, 2025 •

edited

Loading