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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimized tag of exception for tracer. #4082

Merged
merged 6 commits into from Sep 21, 2021

Conversation

limingxinleo
Copy link
Member

No description provided.

@limingxinleo
Copy link
Member Author

@limingxinleo
Copy link
Member Author

@jcchavezs Hi, could you please help me to review this pr?

@@ -18,7 +18,7 @@
'redis' => env('TRACER_ENABLE_REDIS', false),
'db' => env('TRACER_ENABLE_DB', false),
'method' => env('TRACER_ENABLE_METHOD', false),
'error' => false,
'exception' => env('TRACER_ENABLE_EXCEPTION', false),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is TRACER_RECORD_EXCEPTION a more accurate name?

Copy link
Member Author

Choose a reason for hiding this comment

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

I also think TRACER_RECORD_EXCEPTION is more appropriate, but in order to be consistent with other environments, it is better to use TRACER_ENABLE_EXCEPTION .

@@ -60,9 +60,12 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
});
try {
$response = $handler->handle($request);
$span->setTag('response.statusCode', $response->getStatusCode());
$span->setTag('response.status_code', $response->getStatusCode());
Copy link
Contributor

Choose a reason for hiding this comment

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

馃憤

if ($exception instanceof HttpException) {
$span->setTag('error.statusCode', $exception->getStatusCode());
}
$span->setTag('exception.code', $exception->getCode());
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we were not using . as delimiter anymore. Can't we use _ as discussed in #4045 (comment)?

@limingxinleo
Copy link
Member Author

@leocavalcante Hi, could you please help me to review this pr?

leocavalcante
leocavalcante previously approved these changes Sep 20, 2021
@leocavalcante
Copy link
Member

Looks fine for me, personally I will still need to replace the TracerMiddleware for my own implementation because of OTel and New Relic, but, as @jcchavezs said, if we would stick to the OpenTracing standard for the framework, then @Reasno found a nice link at the previous PR: https://github.com/opentracing/specification/blob/master/semantic_conventions.md

Errored/failed spans should be signaled only by the $span->setTag('error', true) and the Exception details goes as Spans Log Fields. Similar to how HTTPClient is already doing.

@limingxinleo
Copy link
Member Author

@leocavalcante

But I think exception.class and exception.code can help us to locate errors and statistical data. According to the logs which saved in elasticsearch and so on, this kind of serious problems can be quickly reflected.

So I set tags about them.

@leocavalcante
Copy link
Member

leocavalcante commented Sep 20, 2021

@limingxinleo

I prefer them too. Actually, span logs didn't work for my environment (zipkin -> otel collector -> new relic) that is why I added all about the Exception as tags.

$span->setTag($this->spanTagManager->get('exception', 'class'), get_class($exception));
$span->setTag($this->spanTagManager->get('exception', 'code'), $exception->getCode());
$span->setTag($this->spanTagManager->get('exception', 'message'), $exception->getMessage());
$span->log(['exception' => (string) $exception]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Still not sure about the log. Could we elaborate more on why it is better to add it as a log?

$span->setTag($this->spanTagManager->get('exception', 'class'), get_class($exception));
$span->setTag($this->spanTagManager->get('exception', 'code'), $exception->getCode());
$span->setTag($this->spanTagManager->get('exception', 'message'), $exception->getMessage());
$span->setTag($this->spanTagManager->get('exception', 'stack_trace'), (string) $exception);
Copy link
Member Author

Choose a reason for hiding this comment

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

@jcchavezs @leocavalcante

Does it meet your needs?

Copy link
Member

Choose a reason for hiding this comment

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

Looks fine! Thanks

@limingxinleo limingxinleo merged commit b54125f into hyperf:master Sep 21, 2021
@limingxinleo limingxinleo deleted the 2.2-tracer branch September 21, 2021 09:06
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.

None yet

3 participants