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

Opentelemetry fixes #4966

Merged
merged 3 commits into from
Nov 28, 2023
Merged

Conversation

zeeshanakram3
Copy link
Contributor

@zeeshanakram3 zeeshanakram3 commented Nov 17, 2023

This PR

  • Fixes loading env vars from .env file in @joystream/opentelemetry module. Due to this bug the OTEL_APPLICATION was not exported and hence DefaultInstrumentation was used for Argus, and hence argus related attributes were not exported to ES.
  • make OTEL maxQueueSize & maxExportBatchSize span processor options configurable
  • Add 'nodeId' attribute to all Argus spans

@mnaamani mnaamani added the argus Argus distributor node label Nov 20, 2023
import { ClientRequest, ServerResponse } from 'http'

/** Opentelemetry Instrumentation for Joystream Distributor Node */

class CustomSpanProcessor extends BatchSpanProcessor {
onStart(span: Span) {
span.setAttribute('nodeId', process.env.JOYSTREAM_DISTRIBUTOR__ID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: I see the code in the config parser setting this env variable, but it happens in the distributor code. Do we know that onStart() is called after the config parser has done it's job?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, the instrument code is loaded & initialized first and then the config parser will do its job, i.e. exporting the env vars. Because of this reason the first few spans when the distributor node starts will have nodeId as undefined (if the config was set through config.yml file and not as env vars).

Copy link
Contributor

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

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

Just left a question, otherwise LGTM.

@mnaamani mnaamani self-requested a review November 24, 2023 14:30
@mnaamani mnaamani merged commit 2094c4f into Joystream:master Nov 28, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
argus Argus distributor node
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants