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

Instrumentation using OTEL #131

Merged
merged 24 commits into from
Oct 5, 2024
Merged

Instrumentation using OTEL #131

merged 24 commits into from
Oct 5, 2024

Conversation

heemankv
Copy link
Contributor

@heemankv heemankv commented Sep 26, 2024

This PR adds Instrumentation to the orchestrator codebase.

  • Adds a file telemetry.rs containing configs and initialisation files for telemetry data.
  • used tokio tracing for adding tracing instrumentation.
  • It is to note that although #instrumentation is added to almost all the functions, they are yet commented, and will only be uncommented as per tracing demands.
  • Tracing Queue size is currently 10000, this can be decreased based on optimal usage.
  • Metrics are added to 'general_/create/process/verify_job` to let know what block is in which state, more metrics can be added upon requirement.
  • While tracing it's best to ignore heavy parameters and only keep small//useful params for analytics, you'll notice that config is always skipped.

EDIT

  • Following YAGNI, removed the commented instrumentations.

@heemankv heemankv self-assigned this Sep 26, 2024
@apoorvsadana apoorvsadana marked this pull request as draft September 26, 2024 04:30
@heemankv heemankv marked this pull request as ready for review September 30, 2024 03:57
@coveralls
Copy link

coveralls commented Sep 30, 2024

Coverage Status

coverage: 64.061% (-0.8%) from 64.811%
when pulling 488a9ef on feature/instrumentation
into ab66b83 on main.

@heemankv
Copy link
Contributor Author

heemankv commented Sep 30, 2024

Completed Self Review, resolved OTEL Vars to ENV and Run OTEL only when env var is present.

crates/da-clients/ethereum/src/config.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/database/mongodb/mod.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/jobs/da_job/mod.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/jobs/mod.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/jobs/mod.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/jobs/mod.rs Show resolved Hide resolved
crates/orchestrator/src/main.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/main.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/telemetry.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/telemetry.rs Outdated Show resolved Hide resolved
@heemankv heemankv linked an issue Oct 1, 2024 that may be closed by this pull request
crates/orchestrator/src/jobs/mod.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/jobs/mod.rs Show resolved Hide resolved
crates/orchestrator/src/jobs/mod.rs Show resolved Hide resolved
crates/orchestrator/src/jobs/da_job/mod.rs Outdated Show resolved Hide resolved
crates/utils/src/env_utils.rs Outdated Show resolved Hide resolved
crates/orchestrator/src/main.rs Outdated Show resolved Hide resolved
Comment on lines 30 to 38
static METER_PROVIDER: Lazy<Arc<SdkMeterProvider>> = Lazy::new(|| {
let meter_provider = init_metric_provider();
Arc::new(meter_provider)
});

static TRACER: Lazy<Arc<Tracer>> = Lazy::new(|| {
let tracer = init_tracer_provider();
Arc::new(tracer)
});
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to still store these?

Copy link
Contributor Author

@heemankv heemankv Oct 5, 2024

Choose a reason for hiding this comment

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

Resolved here.

We have to keep both,
TRACER is used while setting for tracing.
global_meter / METRIC_PROVIDER is needed to get the SdkMetricProvider to be used to shutdown the instrumentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

discusedd to return these from functions

@apoorvsadana apoorvsadana merged commit 7b85453 into main Oct 5, 2024
9 checks passed
ocdbytes pushed a commit that referenced this pull request Oct 16, 2024
* update: telemetry metric + tracing

* update: linting and CI fixes

* update: otel env

* update: run collector only if env is available

* update: correcting otel init

* update: removed commented instrumentation

* update: generalised db_call_type to function_type

* update: otel_enabled, tracing_level, removed tracing logs

* update: removed queue/batch size setter + STDOUT code

* update: added TRACING_LEVEL to env:

* update: fn instrumentation name correction

* update: simplifying Level from string logic

* update: cleaner + generalised impl for Metrics

* update: cleaning new analytics approach

* update: correct names for recording otel

* update: fixing clippy error

* update: adding Sigonz Dashboard

* update: tests for otel

* update: test cases for telemetry

* update: PR comments resolved

* update: cleaned the analytics level

* update: lint fix

* update: CI fixed
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.

Analytics : Instrumentation addition
3 participants