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

Proof of concept for OTel tracing #1676

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Proof of concept for OTel tracing #1676

wants to merge 4 commits into from

Conversation

punya
Copy link

@punya punya commented Feb 6, 2024

Screenshot 2024-02-06 2 57 31 PM

This only covers non-queued execute requests. If the direction makes sense, we could

  • add support for other request types,
  • improve code organization,
  • add tests,
  • add documentation.

Fixes #727.

@punya punya marked this pull request as draft February 6, 2024 20:05
Copy link
Member

@otoolep otoolep left a comment

Choose a reason for hiding this comment

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

The Trace you showed me looks about right. Clustered systems running on local host do about 10-100 INSERTs per second (because the disks saturate for all the nodes). rqlite won't win any prizes for speed.

@@ -24,6 +24,7 @@ message Request {
bool transaction = 1;
repeated Statement statements = 2;
int64 dbTimeout = 3;
map<string, string> metadata = 4;
Copy link
Member

Choose a reason for hiding this comment

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

So why do this? I am wondering if I had use context objects properly and throughout rqlite (I started coding rqlite long before contexts were introduced) would this be necessary?

Copy link
Author

@punya punya Feb 7, 2024

Choose a reason for hiding this comment

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

Yes it would still be necessary.

Context objects propagate trace context within a process; this mechanism is analogous to HTTP headers such as the W3C trace-parent header, and is used to propagate context across process boundaries.

@@ -206,6 +206,9 @@ type Config struct {

// MemProfile enables memory profiling.
MemProfile string

// OTLPDest sets the OTLP endpoint
OTLPDest string
Copy link
Member

Choose a reason for hiding this comment

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

Just so we're clear, what did you set this to at launch time?

Copy link
Author

Choose a reason for hiding this comment

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

$ cat << EOF > config.yml
receivers:
  otlp:
    protocols:
      grpc:
exporters:
  googlecloud:
    project: some-project-name
service:
  pipelines:
    traces:
      receivers: [otlp]
      exporters: [googlecloud]
EOF

$ otelcol-contrib \
  --config ./config.yml &

$ OTEL_SERVICE_NAME=rqlited.1 go run ./cmd/rqlited \
  --otlp-dest localhost:4317 \
  ~/node1 &

$ OTEL_SERVICE_NAME=rqlited.2 go run ./cmd/rqlited \
  --http-addr localhost:14001 \
  --raft-addr localhost:14002 \
  --otlp-dest localhost:4317 \
  --join localhost:4002 \
  ~/node2 &

$ OTEL_SERVICE_NAME=rqlite go run ./cmd/rqlite \
  --otlp-dest localhost:4317

Copy link
Author

Choose a reason for hiding this comment

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

I should mention that this command-line argument is short and explicit, but doesn't expose the full feature set of the OTLP exporter. Before this PR is merged, I'd want to switch over to the autoexport package which is configured using these environment variables.

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.

Add instrumentation via OpenTelemetry
2 participants