Skip to content

Commit

Permalink
fix nestjs sdk requiring endpoint to return (#8276)
Browse files Browse the repository at this point in the history
## Summary

* Updates node.js SDK export batch settings to help with dropped spans
* Fixes nestjs sdk crashing on endpoints where there was no returned
data
* Improves nestjs spans and context propagation for parent->child spans
* Renames nodejs internal spans to make it easier to filter them out.

8844ac6 should fix HIG-4518

## How did you test this change?

![Screenshot from 2024-04-16
22-20-14](https://github.com/highlight/highlight/assets/1351531/d6b9fcb5-72cc-478c-82cb-0e93e71fcc36)

![Screenshot from 2024-04-17
11-27-06](https://github.com/highlight/highlight/assets/1351531/e7aec035-0a66-4fad-b6e3-228887d0351d)

![Screenshot from 2024-04-17
11-34-10](https://github.com/highlight/highlight/assets/1351531/942c0859-bc33-43fd-be63-3f981b2a0064)


## Are there any deployment considerations?

changeset

## Does this work require review from our design team?

no
  • Loading branch information
Vadman97 committed Apr 17, 2024
1 parent 639558b commit ec466b8
Show file tree
Hide file tree
Showing 18 changed files with 195 additions and 107 deletions.
5 changes: 5 additions & 0 deletions .changeset/chatty-adults-hammer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@highlight-run/node': minor
---

refactor highlight-run-with-headers to highlight-ctx
5 changes: 5 additions & 0 deletions .changeset/curly-lobsters-bathe.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@highlight-run/node': patch
---

update node sdk to export in larger batches
6 changes: 6 additions & 0 deletions .changeset/fuzzy-ties-repair.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@highlight-run/nest': minor
'@highlight-run/node': minor
---

fix nestjs trace context propagation and update node sdk span names
5 changes: 5 additions & 0 deletions .changeset/stale-poems-think.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@highlight-run/nest': patch
---

fix nest sdk breaking endpoints that do not return
4 changes: 2 additions & 2 deletions backend/clickhouse/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func (client *Client) WriteErrorGroups(ctx context.Context, groups []*model.Erro
NewStruct(new(ClickhouseErrorGroup)).
InsertInto(ErrorGroupsTable, chGroups...).
BuildWithFlavor(sqlbuilder.ClickHouse)
sql, args = replaceTimestampInserts(sql, args, 10, map[int]bool{1: true, 2: true}, MicroSeconds)
sql, args = replaceTimestampInserts(sql, args, map[int]bool{1: true, 2: true}, MicroSeconds)
return client.conn.Exec(chCtx, sql, args...)
}

Expand Down Expand Up @@ -157,7 +157,7 @@ func (client *Client) WriteErrorObjects(ctx context.Context, objects []*model.Er
NewStruct(new(ClickhouseErrorObject)).
InsertInto(ErrorObjectsTable, chObjects...).
BuildWithFlavor(sqlbuilder.ClickHouse)
sql, args = replaceTimestampInserts(sql, args, 14, map[int]bool{1: true}, MicroSeconds)
sql, args = replaceTimestampInserts(sql, args, map[int]bool{1: true}, MicroSeconds)
return client.conn.Exec(chCtx, sql, args...)
}

Expand Down
12 changes: 8 additions & 4 deletions backend/clickhouse/insert.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"regexp"
"slices"
"strings"
)

type TimeUnit uint8
Expand All @@ -15,14 +16,17 @@ const (
NanoSeconds TimeUnit = 9
)

var pattern = regexp.MustCompile(`\?`)
var keysPattern = regexp.MustCompile(`^INSERT INTO \w+ \(([^)]+)\) VALUES`)
var argPattern = regexp.MustCompile(`\?`)

// replaceTimestampInserts updates direct timestamp inserts to accept int64 unix values
func replaceTimestampInserts(sql string, args []interface{}, numColumns int, columnsToReplace map[int]bool, scale TimeUnit) (string, []interface{}) {
func replaceTimestampInserts(sql string, args []interface{}, columnsToReplace map[int]bool, scale TimeUnit) (string, []interface{}) {
keysMatch := keysPattern.FindStringSubmatch(sql)
keys := strings.Split(keysMatch[1], ",")
var replaced, found int
sql = pattern.ReplaceAllStringFunc(sql, func(s string) string {
sql = argPattern.ReplaceAllStringFunc(sql, func(s string) string {
defer func() { found++ }()
idx := found % numColumns
idx := found % len(keys)
if !columnsToReplace[idx] {
return "?"
}
Expand Down
2 changes: 1 addition & 1 deletion backend/clickhouse/insert_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ func Benchmark_replaceTimestampInserts(b *testing.B) {
for i := 0; i < b.N; i++ {
replaceTimestampInserts("INSERT INTO error_groups (ProjectID, CreatedAt, UpdatedAt, ID, Event, Status, Type, ErrorTagID, ErrorTagTitle, ErrorTagDescription) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", []interface{}{
1, ts, ts + 1, 2, "event", "status", "type", 3, "title", "description",
}, 10, map[int]bool{0: true, 1: true, 2: true, 7: true, 9: true}, NanoSeconds)
}, map[int]bool{0: true, 1: true, 2: true, 7: true, 9: true}, NanoSeconds)
}
assert.Less(b, b.Elapsed()/time.Duration(b.N), time.Millisecond)
}
4 changes: 2 additions & 2 deletions backend/clickhouse/sessions.go
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ func (client *Client) WriteSessions(ctx context.Context, sessions []*model.Sessi
NewStruct(new(ClickhouseSession)).
InsertInto(SessionsTable, chSessions...).
BuildWithFlavor(sqlbuilder.ClickHouse)
sessionsSql, sessionsArgs = replaceTimestampInserts(sessionsSql, sessionsArgs, 34, map[int]bool{7: true, 8: true}, MicroSeconds)
sessionsSql, sessionsArgs = replaceTimestampInserts(sessionsSql, sessionsArgs, map[int]bool{7: true, 8: true}, MicroSeconds)
return client.conn.Exec(chCtx, sessionsSql, sessionsArgs...)
})
}
Expand All @@ -228,7 +228,7 @@ func (client *Client) WriteSessions(ctx context.Context, sessions []*model.Sessi
NewStruct(new(ClickhouseField)).
InsertInto(FieldsTable, chFields...).
BuildWithFlavor(sqlbuilder.ClickHouse)
fieldsSql, fieldsArgs = replaceTimestampInserts(fieldsSql, fieldsArgs, 6, map[int]bool{3: true}, MicroSeconds)
fieldsSql, fieldsArgs = replaceTimestampInserts(fieldsSql, fieldsArgs, map[int]bool{3: true}, MicroSeconds)
return client.conn.Exec(chCtx, fieldsSql, fieldsArgs...)
})
}
Expand Down
2 changes: 1 addition & 1 deletion docs-content/sdk/nodejs.md
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ slug: nodejs
<section className="section">
<div className="left">
<h3>H.runWithHeaders</h3>
<p>H.runWithHeaders() wraps its callback with a span named highlight-run-with-headers, and spreads Highlight's secureSessionId and requestId across all spans with matching a traceId.</p>
<p>H.runWithHeaders() wraps its callback with a span named highlight-ctx, and spreads Highlight's secureSessionId and requestId across all spans with matching a traceId.</p>
<h6>Method Parameters</h6>
<aside className="parameter">
<h5>headers<code>IncomingHttpHeaders</code> <code>required</code></h5>
Expand Down
19 changes: 14 additions & 5 deletions e2e/express-ts/src/express.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import express from 'express'
import { H, Handlers } from '@highlight-run/node'
import { CONSTANTS } from './constants'
import { config } from './instrumentation'
import { context } from '@opentelemetry/api'

H.init(config)

Expand All @@ -11,17 +12,25 @@ const port = 3003
// This should be before any controllers (route definitions)
app.use(Handlers.middleware(config))
app.get('/', async (req, res) => {
await H.runWithHeaders(req.headers, () => {
await H.runWithHeaders(req.headers, async () => {
const err = new Error('this is a test error', {
cause: { route: '/', foo: ['bar'] },
})
console.info('Sending error to highlight')
H.consumeError(err)
H.consumeError(
new Error('this is another test error', {
cause: 'bad code',
}),
const { span, ctx } = H.startWithHeaders(
'second-error',
req.headers,
{},
)
context.with(ctx, () => {
H.consumeError(
new Error('this is another test error', {
cause: 'bad code',
}),
)
})
span.end()

res.send('Hello World!')
})
Expand Down
9 changes: 7 additions & 2 deletions e2e/nestjs/src/app.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,16 @@ export class AppController {

@Get()
async getHello(): Promise<string[]> {
return await this.appService.findAll();
return await this.appService.findAll({});
}

@Get('/empty')
async getEmpty() {
return await this.appService.findAll({ empty: true });
}

@Get('/error')
async getError(): Promise<string[]> {
return await this.appService.findAll(true);
return await this.appService.findAll({ error: true });
}
}
28 changes: 23 additions & 5 deletions e2e/nestjs/src/app.service.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,21 @@
import { Injectable, Logger } from '@nestjs/common';
import { firstValueFrom } from 'rxjs';
import { HttpService } from '@nestjs/axios';
import { H } from '@highlight-run/nest';

@Injectable()
export class AppService {
private readonly logger = new Logger(AppService.name);
constructor(private readonly httpService: HttpService) {}

async findAll(throwError: boolean = false): Promise<string[]> {
async findAll({
error,
empty,
}: {
error?: true;
empty?: true;
}): Promise<string[]> {
const span = await H.startActiveSpan('making request now', {});
await firstValueFrom(
this.httpService.post<any[]>(
'https://pub.highlight.io/v1/logs/json',
Expand All @@ -24,13 +32,23 @@ export class AppService {
},
),
);
span.end();

this.logger.log('hello, world!');
this.logger.warn('whoa there! ', Math.random());
if (throwError) {
for (let i = 0; i < 10; i++) {
const s = await H.startActiveSpan(`another request ${i}`, {
attributes: { i },
});
console.log('hello, world!');
this.logger.log('hello, world!');
this.logger.warn('whoa there! ', Math.random());
s.end();
}
if (error) {
// error will be caught by the HighlightErrorFilter
throw new Error(`a random error occurred!`);
}
return [`Hello World!`];
if (!empty) {
return [`Hello World!`];
}
}
}
2 changes: 1 addition & 1 deletion e2e/nestjs/src/main.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { NestFactory } from '@nestjs/core';
import { AppModule } from './app.module';
import { HighlightInterceptor, H } from '@highlight-run/nest';

const env = {
projectID: '2',
Expand All @@ -11,6 +10,7 @@ const env = {
};

async function bootstrap() {
const { HighlightInterceptor, H } = await import('@highlight-run/nest');
H.init(env);
const app = await NestFactory.create(AppModule);
app.useGlobalInterceptors(new HighlightInterceptor(env));
Expand Down
2 changes: 1 addition & 1 deletion sdk/highlight-apollo/src/apollo.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe('ApolloServerHighlightPlugin', () => {
const { details } = await getResourceSpans(port)

const highlightRunWithHeadersSpan = details.find(({ spanNames }) =>
spanNames.includes('highlight-run-with-headers'),
spanNames.includes('highlight-ctx'),
)
const sessionId =
highlightRunWithHeadersSpan?.attributes['highlight.session_id']
Expand Down
2 changes: 1 addition & 1 deletion sdk/highlight-apollo/src/apolloV3.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ describe('ApolloServerV3HighlightPlugin', () => {
const { details } = await getResourceSpans(port)

const highlightRunWithHeadersSpan = details.find(({ spanNames }) =>
spanNames.includes('highlight-run-with-headers'),
spanNames.includes('highlight-ctx'),
)
const sessionId =
highlightRunWithHeadersSpan?.attributes['highlight.session_id']
Expand Down
59 changes: 31 additions & 28 deletions sdk/highlight-nest/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ import {
Injectable,
NestInterceptor,
} from '@nestjs/common'
import { lastValueFrom, Observable, throwError } from 'rxjs'
import { finalize, Observable, throwError } from 'rxjs'
import { catchError } from 'rxjs/operators'
import api from '@opentelemetry/api'

@Injectable()
export class HighlightLogger
Expand Down Expand Up @@ -93,36 +94,38 @@ export class HighlightInterceptor
await NodeH.flush()
}

intercept(
context: ExecutionContext,
next: CallHandler,
): Promise<Observable<any>> {
intercept(context: ExecutionContext, next: CallHandler): Observable<any> {
const ctx = context.switchToHttp()
const request = ctx.getRequest()
const highlightCtx = NodeH.parseHeaders(request.headers)
return NodeH.runWithHeaders(request.headers, async () => {
const span = await NodeH.startActiveSpan(
`${request.method} ${request.url}`,
{
attributes: {
'http.method': request.method,
'http.url': request.url,
},

const { span: requestSpan, ctx: spanCtx } = NodeH.startWithHeaders(
`${request.method} ${request.url}`,
request.headers,
{
attributes: {
'http.method': request.method,
'http.url': request.url,
},
)
try {
return await lastValueFrom(next.handle())
} catch (err: any) {
NodeH.consumeError(
err,
highlightCtx?.secureSessionId,
highlightCtx?.requestId,
)
throw err
} finally {
span.end()
}
})
},
)
const fn = api.context.bind(spanCtx, () =>
next.handle().pipe(
catchError((err) => {
NodeH.consumeError(
err,
undefined,
undefined,
{},
{ span: requestSpan },
)
return throwError(() => err)
}),
finalize(() => {
requestSpan.end()
}),
),
)
return fn()
}
}

Expand Down
Loading

0 comments on commit ec466b8

Please sign in to comment.