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

Autocomplete: Improve sampling code and prepare for Honeycomb export #3034

Merged
merged 21 commits into from
Feb 9, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/shared/src/experimentation/FeatureFlagProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ export class FeatureFlagProvider {
return this.featureFlags[endpoint]?.[flagName]
}

public getExposedExperiments(endpoint: string = this.apiClient.endpoint): Record<string, boolean> {
return this.featureFlags[endpoint] || {}
}

public async evaluateFeatureFlag(flagName: FeatureFlag): Promise<boolean> {
const endpoint = this.apiClient.endpoint
if (process.env.BENCHMARK_DISABLE_FEATURE_FLAGS) {
Expand Down
5 changes: 0 additions & 5 deletions lib/shared/src/sourcegraph-api/graphql/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -776,11 +776,6 @@ export class SourcegraphGraphQLAPIClient {
headers,
})
.then(verifyResponseCode)
//.then(response => response.text())
//.then(text => {
// console.log('fetched:', text)
// return JSON.parse(text) as T
//})
valerybugakov marked this conversation as resolved.
Show resolved Hide resolved
.then(response => response.json() as T)
.catch(error => {
return new Error(`accessing Sourcegraph GraphQL API: ${error} (${url})`)
Expand Down
25 changes: 16 additions & 9 deletions lib/shared/src/tracing/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import opentelemetry, { SpanStatusCode, context, propagation, type Exception } from '@opentelemetry/api'
import opentelemetry, {
SpanStatusCode,
context,
propagation,
type Exception,
Span,
} from '@opentelemetry/api'

const INSTRUMENTATION_SCOPE_NAME = 'cody'
const INSTRUMENTATION_SCOPE_VERSION = '0.1'
Expand All @@ -17,31 +23,32 @@ export function getActiveTraceAndSpanId(): { traceId: string; spanId: string } |
return undefined
}

export function wrapInActiveSpan<R>(name: string, fn: () => R): R {
export function wrapInActiveSpan<R>(name: string, fn: (span: Span) => R): R {
return tracer.startActiveSpan(name, (span): R => {
const handleSuccess = (response: R): R => {
span.setStatus({ code: SpanStatusCode.OK })
span.end()
return response
}

const catchError = (error: unknown): never => {
const handleError = (error: unknown): never => {
span.recordException(error as Exception)
span.setStatus({ code: SpanStatusCode.ERROR })
span.end()
throw error
}

try {
const response = fn()
const response = fn(span)

if (response instanceof Promise) {
return response.then(handleSuccess, catchError) as R
if (typeof response === 'object' && response !== null && 'then' in response) {
// @ts-ignore Response seems to be a Thenable
return response.then(handleSuccess, handleError) as R
}

return handleSuccess(response)
} catch (error) {
return catchError(error)
} finally {
span.end()
return handleError(error)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was causing the issue we saw with traces because for async responses, span.end would be called synchronously after creating the promise but before it resolved which left all spans be very short 😬

Copy link
Member

Choose a reason for hiding this comment

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

would be called synchronously after creating the promise but before it resolved, which left all spans be very short

TY for catching this! Makes sense.

}
})
}
Expand Down
Loading
Loading