Set span status according to Semantic Conventions of Http #164

gnm444 · 2022-05-24T10:41:38Z

Issue : https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#Status

linux-foundation-easycla · 2022-05-24T10:41:41Z

The committers listed above are authorized under a signed CLA.

✅ login: gnm444 / name: Narasimha (8b8b4a1, 4a37f8c, 5b030f2)

DebajitDas · 2022-05-24T12:26:07Z

instrumentation/otel-webserver-module/include/core/sdkwrapper/IScopedSpan.h

+enum class SpanType {
+  INTERNAL,
+  SERVER,
+  CLIENT
 };


We already have SpanKind defined in ISdkWrapper. Any Challenge in re-using that?

Since they are in ISdkWrapper.h, including that in this header file is leading to circular inclusions. So avoided it.

ISdkWrapper is the parent right? Why would there be a problem of circular inclusion?

Moved them to a new file to avoid circular inclusion

DebajitDas · 2022-05-24T12:42:24Z

instrumentation/otel-webserver-module/src/core/api/RequestProcessingEngine.cpp



 namespace appd {
 namespace core {

+#define HTTP_ERROR_1XX 100
+#define HTTP_ERROR_4XX 400
+#define HTTP_ERROR_5XX 500


Just a discussion, Is it not better to use constexpr instead ?

Yes Const is better. I will change to const int.

or we can use constexpr https://en.cppreference.com/w/cpp/language/constexpr as well.

3 defines look a little clumsy. Should we use a different data structure(like a struct)?

Addressed this by using constexpr in SdkConstants.h

DebajitDas · 2022-05-30T09:35:24Z

instrumentation/otel-webserver-module/include/core/sdkwrapper/SdkEnums.h

+
+} // sdkwrapper
+} // core
+} // appd


Add a newline

instrumentation/otel-webserver-module/src/core/api/RequestProcessingEngine.cpp

kpratyus · 2022-05-25T17:35:10Z

instrumentation/otel-webserver-module/src/core/api/RequestProcessingEngine.cpp

+            interactionSpan->SetStatus(sdkwrapper::StatusCode::Unset);
+        }
+        else if (payload->errorCode >= HTTP_ERROR_4XX &&   payload->errorCode < HTTP_ERROR_5XX ) {
+            if (interactionSpan->GetSpanKind() == sdkwrapper::SpanType::SERVER)


@DebajitDas just a question. The error and status code should be once for the whole request processing phase. Do we need to set the status for every span?

kpratyus · 2022-05-25T17:37:12Z

instrumentation/otel-webserver-module/src/core/sdkwrapper/ScopedSpan.cpp

@@ -32,6 +32,7 @@ ScopedSpan::ScopedSpan(
 	options.kind = kind;
 	mSpan = sdkHelperFactory->GetTracer()->StartSpan(name, attributes, options);
 	mScope.reset(new trace::Scope(mSpan));
+	mSpanKind = kind;


Why do we need mSpanKind if we have options.kind?

kpratyus · 2022-05-25T17:39:24Z

instrumentation/otel-webserver-module/src/core/sdkwrapper/ServerSpan.cpp

@@ -68,6 +68,12 @@ void ServerSpan::SetStatus(const StatusCode status, const std::string& desc)
  mScopedSpan->SetStatus(status, desc);
 }

+SpanType ServerSpan::GetSpanKind()
+{


Although the logic is correct, should we refactor the code in a way that both Serverspan and scopedspan have similar implementation for get span kind? This would be good for future as well

kpratyus · 2022-05-25T17:40:39Z

instrumentation/otel-webserver-module/include/core/sdkwrapper/IScopedSpan.h


-  virtual void SetStatus(const StatusCode status, const std::string& desc = "") = 0;
+        virtual SpanType GetSpanKind() = 0;


Is SpanType same as trace::SpanKind?

DebajitDas

LGTM

DebajitDas · 2022-05-31T12:23:47Z

LGTM, will merge once Pratyush approves as well

* Set span status according to http semantics * Set span status changes part-2 * Set span status according to http semantics spaces * Review comments addressed for span status * Moved error status from Apache hooks to Request processing engine Co-authored-by: Narasimha <ngonapa@cisco.com>

* Set span status according to Semantic Conventions of Http (#164) * Set span status according to http semantics * Set span status changes part-2 * Set span status according to http semantics spaces * Review comments addressed for span status * Moved error status from Apache hooks to Request processing engine * Fixed UT failure (#171) * Set span status according to http semantics * Set span status changes part-2 * Set span status according to http semantics spaces * Review comments addressed for span status * Moved error status from Apache hooks to Request processing engine * Fixed unit test failure Co-authored-by: Narasimha <ngonapa@cisco.com>

* Added dependencies of Nginx Build (#158) * updated nginx build directory (#159) * nginx source code (#160) * Add license (#162) * Add license * removing Ubuntu condition * Nomenclature changes from Appdynamics to OpenTelemetry (#163) * Changed appd to otel * Added build command for Nginx Module * Added a missing file to be built * Incorporated Review comments * Nginx docker (#167) * Changing docker compose * centos7 * Updated README for Nginx instrumentation (#168) * Updated README for Nginx instrumentation * Updated README.md * Updated README.md * Set span status according to Semantic Conventions of Http (#164) (#169) * Set span status according to http semantics * Set span status changes part-2 * Set span status according to http semantics spaces * Review comments addressed for span status * Moved error status from Apache hooks to Request processing engine Co-authored-by: Narasimha <ngonapa@cisco.com> * Updated error codes (#170) * Merged from Main branch (#172) * Set span status according to Semantic Conventions of Http (#164) * Set span status according to http semantics * Set span status changes part-2 * Set span status according to http semantics spaces * Review comments addressed for span status * Moved error status from Apache hooks to Request processing engine * Fixed UT failure (#171) * Set span status according to http semantics * Set span status changes part-2 * Set span status according to http semantics spaces * Review comments addressed for span status * Moved error status from Apache hooks to Request processing engine * Fixed unit test failure Co-authored-by: Narasimha <ngonapa@cisco.com> * Merge from main (#181) * Set span status according to Semantic Conventions of Http (#164) * Set span status according to http semantics * Set span status changes part-2 * Set span status according to http semantics spaces * Review comments addressed for span status * Moved error status from Apache hooks to Request processing engine * Fixed UT failure (#171) * Set span status according to http semantics * Set span status changes part-2 * Set span status according to http semantics spaces * Review comments addressed for span status * Moved error status from Apache hooks to Request processing engine * Fixed unit test failure Co-authored-by: Narasimha <ngonapa@cisco.com> Co-authored-by: DEBAJIT DAS <85024550+DebajitDas@users.noreply.github.com> Co-authored-by: Narasimha <ngonapa@cisco.com>

…cation. (#174) * Added dependencies of Nginx Build (#158) * updated nginx build directory (#159) * nginx source code (#160) * Add license (#162) * Add license * removing Ubuntu condition * Nomenclature changes from Appdynamics to OpenTelemetry (#163) * Changed appd to otel * Added build command for Nginx Module * Added a missing file to be built * Incorporated Review comments * Nginx docker (#167) * Changing docker compose * centos7 * Updated README for Nginx instrumentation (#168) * Updated README for Nginx instrumentation * Updated README.md * Updated README.md * Set span status according to Semantic Conventions of Http (#164) (#169) * Set span status according to http semantics * Set span status changes part-2 * Set span status according to http semantics spaces * Review comments addressed for span status * Moved error status from Apache hooks to Request processing engine Co-authored-by: Narasimha <ngonapa@cisco.com> * Updated error codes (#170) * Merged from Main branch (#172) * Set span status according to Semantic Conventions of Http (#164) * Set span status according to http semantics * Set span status changes part-2 * Set span status according to http semantics spaces * Review comments addressed for span status * Moved error status from Apache hooks to Request processing engine * Fixed UT failure (#171) * Set span status according to http semantics * Set span status changes part-2 * Set span status according to http semantics spaces * Review comments addressed for span status * Moved error status from Apache hooks to Request processing engine * Fixed unit test failure Co-authored-by: Narasimha <ngonapa@cisco.com> * Handling span creation/end on Nginx internal redirection and named location, related to bug WEBSRV-721 * Fix for try files modules * Added minor changes * Incorporated some review comments Co-authored-by: Kumar Pratyush <95214718+kpratyus@users.noreply.github.com> Co-authored-by: Narasimha <ngonapa@cisco.com>

gnm444 added 3 commits May 24, 2022 11:30

Set span status according to http semantics

8b8b4a1

Set span status changes part-2

4a37f8c

Set span status according to http semantics spaces

5b030f2

gnm444 requested a review from DebajitDas as a code owner May 24, 2022 10:41

gnm444 requested a review from a team May 24, 2022 10:41

DebajitDas requested a review from kpratyus May 24, 2022 10:51

DebajitDas requested changes May 24, 2022

View reviewed changes

Review comments addressed for span status

490a33e

DebajitDas reviewed May 30, 2022

View reviewed changes

Moved error status from Apache hooks to Request processing engine

2f7b6f5

kpratyus reviewed May 31, 2022

View reviewed changes

DebajitDas self-requested a review May 31, 2022 12:22

DebajitDas approved these changes May 31, 2022

View reviewed changes

kpratyus approved these changes Jun 1, 2022

View reviewed changes

DebajitDas merged commit d1b9c1e into open-telemetry:main Jun 1, 2022

gnm444 mentioned this pull request Mar 30, 2023

REQUEST: New membership for <gnm444> open-telemetry/community#1402

Closed

marcalff added the Webserver This represents the otel-webserver-module in the instrumentation directory label May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set span status according to Semantic Conventions of Http #164

Set span status according to Semantic Conventions of Http #164

gnm444 commented May 24, 2022

linux-foundation-easycla bot commented May 24, 2022 •

edited

Loading

DebajitDas May 24, 2022

gnm444 May 25, 2022

kpratyus May 25, 2022

gnm444 May 27, 2022

DebajitDas May 24, 2022

gnm444 May 25, 2022

DebajitDas May 25, 2022

kpratyus May 25, 2022

gnm444 May 27, 2022

DebajitDas May 30, 2022

gnm444 May 30, 2022

kpratyus May 25, 2022

kpratyus May 25, 2022

kpratyus May 25, 2022

kpratyus May 25, 2022

DebajitDas left a comment

DebajitDas commented May 31, 2022


		virtual void SetStatus(const StatusCode status, const std::string& desc = "") = 0;
		virtual SpanType GetSpanKind() = 0;

Set span status according to Semantic Conventions of Http #164

Set span status according to Semantic Conventions of Http #164

Conversation

gnm444 commented May 24, 2022

linux-foundation-easycla bot commented May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DebajitDas left a comment

Choose a reason for hiding this comment

DebajitDas commented May 31, 2022

linux-foundation-easycla bot commented May 24, 2022 •

edited

Loading