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: add temporal metrics and tracing docs for Signoz #1208

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

feat: add temporal metrics and tracing docs for Signoz #1208

wants to merge 3 commits into from

Conversation

Creator54
Copy link
Contributor

@Creator54 Creator54 commented Mar 4, 2025 •
edited by Calm-Rock
Loading

Fixes: #1156

@vercel Vercel
Copy link

vercel bot commented Mar 4, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
signoz-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 4, 2025 4:19pm

@Creator54 Creator54 force-pushed the temporal-docs branch 2 times, most recently from 0b94a56 to 2d69743 Compare March 10, 2025 01:23
@ankitnayan ankitnayan self-requested a review March 18, 2025 04:31
@ankitnayan
Copy link
Contributor

I will be reviewing this when ready

@ankitnayan
Copy link
Contributor

@Creator54 when will this be ready for review?

@Creator54 Creator54 force-pushed the temporal-docs branch 2 times, most recently from af79a49 to ccc4e84 Compare March 23, 2025 11:16
@Creator54
Copy link
Contributor Author

@Creator54 when will this be ready for review?

this is ready for review, please check and let me know if any changes required.

@Creator54 Creator54 marked this pull request as ready for review March 23, 2025 16:22
@Calm-Rock Calm-Rock self-requested a review March 24, 2025 06:03
@Calm-Rock
Copy link
Member

@Creator54 when will this be ready for review?

@ankitnayan this PR is ready for review - Preview here

@ankitnayan
Copy link
Contributor

@Creator54 which guide/doc did you follow to create code structure at https://github.com/Creator54/temporal-typescript-demo-app/tree/otel/src/config?

@Creator54
Copy link
Contributor Author

@Creator54 which guide/doc did you follow to create code structure at https://github.com/Creator54/temporal-typescript-demo-app/tree/otel/src/config?

I started with the Temporal TypeScript samples and the observability docs, but found them challenging to follow. So decided to port the temporal-java project.

@ankitnayan
Copy link
Contributor

The tracing docs has references to metrics code too. So, the steps mentioned in the traces docs won't run independently.

IMO we should build single docs for setup of Metrics + Traces + Logs

@ankitnayan
Copy link
Contributor

ankitnayan commented Mar 24, 2025 •
edited
Loading

the code at src/worker.ts changes alot with the otel docs. The application code which user has created before adding otel should not change much. The docs should mentions ways to add telemetry with minimal changes to existing application code

Apart from above, the code structure at the otel branch is very different than the application code itself where the user started adding telemetry. It seems like 2 different codebases. Eg, the activities file is changed to the below structure

Screenshot 2025-03-24 at 10 54 17 PM

@ankitnayan
Copy link
Contributor

the outbound and internal interceptors seem missing as they are present in the official example https://github.com/temporalio/samples-typescript/blob/main/interceptors-opentelemetry/src/workflows.ts#L19

@ankitnayan
Copy link
Contributor

I see data hardcoded in the sample app. Why is this so?
https://github.com/Creator54/temporal-typescript-demo-app/blob/otel/src/workers/helloWorldWorker.ts#L51-L61
https://github.com/Creator54/temporal-typescript-demo-app/blob/otel/src/client.ts#L167-L176

@ankitnayan
Copy link
Contributor

Every metric datapoint is being created periodically in a hardcoded way? https://github.com/Creator54/temporal-typescript-demo-app/blob/otel/src/config/temporalConfig.ts#L124-L132

@Creator54
Copy link
Contributor Author

Every metric datapoint is being created periodically in a hardcoded way? https://github.com/Creator54/temporal-typescript-demo-app/blob/otel/src/config/temporalConfig.ts#L124-L132

It was for visualization purposes, so most panels have some data to visualize without waiting for all the flows.

@ankitnayan
Copy link
Contributor

ankitnayan commented Mar 26, 2025 •
edited
Loading

  1. Create single doc for metrics + tracing + logs (later)
  2. Make the doc snippets works and the repo is just for exploration like checking imports, versions and to run and get a feel. A user would want to add those snippets to his application code so there should not be major structural changes between the different branches with and without instrumentation
  3. I tried running the code, it didn't work. Saying namespace doesn't exist though it existed. Please share the exact command + a video running the command (for me to verify, can DM me this)
  4. Clear up the repo of code that generates dummy data. That is very confusing.
  5. Focus on typescript first if possible. I have evaluated that as a couple of users need that. And do not port java docs to typescript using chatgpt. Users would like the follow the official documentation as the converted ones have a lot of changes which are not there in the language official doc
  6. feat: add temporal metrics and tracing docs for Signoz #1208 (comment)

@Creator54 Creator54 marked this pull request as draft March 31, 2025 11:46
ankitnayan
ankitnayan reviewed Apr 4, 2025
View reviewed changes
data/docs/instrumentation/temporal-instrumentation/opentelemetry-temporal-typescript.mdx Outdated
telemetryOptions: {
metrics: {
otel: {
url: process.env.OTEL_EXPORTER_OTLP_ENDPOINT || 'http://127.0.0.1:4317',
Copy link
Contributor

Choose a reason for hiding this comment

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

missing auth headers

          headers: {
            'signoz-ingestion-key': xxx,
          },

data/docs/instrumentation/temporal-instrumentation/opentelemetry-temporal-typescript.mdx Outdated
metrics: {
otel: {
url: process.env.OTEL_EXPORTER_OTLP_ENDPOINT || 'http://127.0.0.1:4317',
metricsExportInterval: '1s'
Copy link
Contributor

Choose a reason for hiding this comment

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

make it 10s

data/docs/instrumentation/temporal-instrumentation/opentelemetry-temporal-typescript.mdx

function setupTraceExporter(): SpanExporter | undefined {
console.log('[Instrumentation] Setting up Trace Exporter to', OTEL_EXPORTER_OTLP_ENDPOINT);
return new OTLPTraceExporterGrpc({
Copy link
Contributor

Choose a reason for hiding this comment

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

missing auth headers

          headers: {
            'signoz-ingestion-key': xxx,
          },

data/docs/instrumentation/temporal-instrumentation/opentelemetry-temporal-typescript.mdx
function setupMetricReader() {
console.log('[Instrumentation] Setting up Metric Reader to', OTEL_EXPORTER_OTLP_ENDPOINT);
return new PeriodicExportingMetricReader({
exporter: new OTLPMetricExporterGrpc({
Copy link
Contributor

Choose a reason for hiding this comment

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

missing auth headers

          headers: {
            'signoz-ingestion-key': xxx,
          },

data/docs/instrumentation/temporal-instrumentation/opentelemetry-temporal-typescript.mdx Outdated
}

export const resource = new Resource({
[ATTR_SERVICE_NAME]: OTEL_RESOURCE_ATTRIBUTES.split(',')[0].split('=')[1],
Copy link
Contributor

Choose a reason for hiding this comment

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

this only parses the 0th element of the array.

  1. SERVICE_NAME might not necessarily be at 0th position
  2. We must parse all attributes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ankitnayan ankitnayan ankitnayan left review comments

@Calm-Rock Calm-Rock Awaiting requested review from Calm-Rock

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[DOCS] Temporal.io metrics to SigNoz
3 participants
@Creator54 @ankitnayan @Calm-Rock