From 02b6e8a73c23301d6c62bc812396548ba97f0274 Mon Sep 17 00:00:00 2001 From: DEBAJIT DAS <85024550+DebajitDas@users.noreply.github.com> Date: Thu, 2 Jun 2022 10:19:17 +0530 Subject: [PATCH] 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 --- .../include/core/sdkwrapper/IScopedSpan.h | 13 +++-- .../include/core/sdkwrapper/ISdkWrapper.h | 7 +-- .../include/core/sdkwrapper/ScopedSpan.h | 7 ++- .../include/core/sdkwrapper/SdkConstants.h | 5 ++ .../include/core/sdkwrapper/SdkEnums.h | 37 ++++++++++++++ .../include/core/sdkwrapper/ServerSpan.h | 2 + .../src/apache/ApacheHooks.cpp | 2 +- .../src/core/api/RequestProcessingEngine.cpp | 51 ++++++++++++++++--- .../src/core/sdkwrapper/ScopedSpan.cpp | 11 ++++ .../src/core/sdkwrapper/ServerSpan.cpp | 6 +++ 10 files changed, 119 insertions(+), 22 deletions(-) create mode 100644 instrumentation/otel-webserver-module/include/core/sdkwrapper/SdkEnums.h diff --git a/instrumentation/otel-webserver-module/include/core/sdkwrapper/IScopedSpan.h b/instrumentation/otel-webserver-module/include/core/sdkwrapper/IScopedSpan.h index 40673c427..d0f992c18 100644 --- a/instrumentation/otel-webserver-module/include/core/sdkwrapper/IScopedSpan.h +++ b/instrumentation/otel-webserver-module/include/core/sdkwrapper/IScopedSpan.h @@ -17,10 +17,12 @@ #ifndef __ISCOPEDSPAN_H #define __ISCOPEDSPAN_H +#include "sdkwrapper/SdkEnums.h" #include #include #include + namespace appd { namespace core { namespace sdkwrapper { @@ -29,11 +31,6 @@ using SpanAttributeValue = opentelemetry::common::AttributeValue; using OtelKeyValueMap = std::unordered_map; using namespace opentelemetry; -enum class StatusCode { - Ok, - Error -}; - class IScopedSpan { public: virtual ~IScopedSpan() = default; @@ -45,9 +42,11 @@ class IScopedSpan { const OtelKeyValueMap& attributes) = 0; virtual void AddAttribute(const std::string& key, - const SpanAttributeValue& value) = 0; + const SpanAttributeValue& value) = 0; + + virtual void SetStatus(const StatusCode status, const std::string& desc = "") = 0; - virtual void SetStatus(const StatusCode status, const std::string& desc = "") = 0; + virtual SpanKind GetSpanKind() = 0; }; } //sdkwrapper diff --git a/instrumentation/otel-webserver-module/include/core/sdkwrapper/ISdkWrapper.h b/instrumentation/otel-webserver-module/include/core/sdkwrapper/ISdkWrapper.h index 3b465219e..4caf8fed7 100644 --- a/instrumentation/otel-webserver-module/include/core/sdkwrapper/ISdkWrapper.h +++ b/instrumentation/otel-webserver-module/include/core/sdkwrapper/ISdkWrapper.h @@ -19,6 +19,7 @@ #include #include +#include "sdkwrapper/SdkEnums.h" #include "sdkwrapper/IScopedSpan.h" #include "api/TenantConfig.h" @@ -26,12 +27,6 @@ namespace appd { namespace core { namespace sdkwrapper { -enum class SpanKind { - INTERNAL, - SERVER, - CLIENT -}; - class ISdkWrapper { public: virtual ~ISdkWrapper() = default; diff --git a/instrumentation/otel-webserver-module/include/core/sdkwrapper/ScopedSpan.h b/instrumentation/otel-webserver-module/include/core/sdkwrapper/ScopedSpan.h index 8c92ad3df..6f76331c8 100644 --- a/instrumentation/otel-webserver-module/include/core/sdkwrapper/ScopedSpan.h +++ b/instrumentation/otel-webserver-module/include/core/sdkwrapper/ScopedSpan.h @@ -46,12 +46,15 @@ class ScopedSpan : public IScopedSpan { void AddAttribute(const std::string& key, const SpanAttributeValue& value) override; - void SetStatus(const StatusCode status, const std::string& desc) override; + void SetStatus(const StatusCode status, const std::string& desc) override; + + SpanKind GetSpanKind(); private: opentelemetry::nostd::shared_ptr mSpan; std::unique_ptr mScope; - const AgentLogger& mLogger; + const AgentLogger& mLogger; + trace::SpanKind mSpanKind; }; } //sdkwrapper diff --git a/instrumentation/otel-webserver-module/include/core/sdkwrapper/SdkConstants.h b/instrumentation/otel-webserver-module/include/core/sdkwrapper/SdkConstants.h index 64f58cd33..b7c436379 100644 --- a/instrumentation/otel-webserver-module/include/core/sdkwrapper/SdkConstants.h +++ b/instrumentation/otel-webserver-module/include/core/sdkwrapper/SdkConstants.h @@ -24,6 +24,11 @@ const std::string kServiceNamespace = "service.namespace"; const std::string kServiceInstanceId = "service.instance.id"; const std::string kOtelLibraryName = "telemetry.sdk.language"; const std::string kOtelLibraryVersion = "telemetry.sdk.version"; +const std::string kHttpErrorCode = "HTTP ERROR CODE:"; +constexpr int HTTP_ERROR_1XX = 100; +constexpr int HTTP_ERROR_4XX = 400; +constexpr int HTTP_ERROR_5XX = 500; + } // sdkwrapper } // core } // appd diff --git a/instrumentation/otel-webserver-module/include/core/sdkwrapper/SdkEnums.h b/instrumentation/otel-webserver-module/include/core/sdkwrapper/SdkEnums.h new file mode 100644 index 000000000..bf75a055c --- /dev/null +++ b/instrumentation/otel-webserver-module/include/core/sdkwrapper/SdkEnums.h @@ -0,0 +1,37 @@ +/* +* Copyright 2021 AppDynamics LLC. +* +* Licensed under the Apache License, Version 2.0 (the "License"); +* you may not use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +*/ + +#pragma once + +namespace appd { +namespace core { +namespace sdkwrapper { + +enum class StatusCode { + Ok, + Error, + Unset +}; + +enum class SpanKind { + INTERNAL, + SERVER, + CLIENT +}; + +} // sdkwrapper +} // core +} // appd diff --git a/instrumentation/otel-webserver-module/include/core/sdkwrapper/ServerSpan.h b/instrumentation/otel-webserver-module/include/core/sdkwrapper/ServerSpan.h index 33b875931..9c2a95c3a 100644 --- a/instrumentation/otel-webserver-module/include/core/sdkwrapper/ServerSpan.h +++ b/instrumentation/otel-webserver-module/include/core/sdkwrapper/ServerSpan.h @@ -51,6 +51,8 @@ class ServerSpan : public IScopedSpan { void SetStatus(const StatusCode status, const std::string& desc) override; + SpanKind GetSpanKind(); + private: std::unique_ptr mScopedSpan; nostd::unique_ptr mToken; diff --git a/instrumentation/otel-webserver-module/src/apache/ApacheHooks.cpp b/instrumentation/otel-webserver-module/src/apache/ApacheHooks.cpp index 1cebd8c0d..efd004efb 100644 --- a/instrumentation/otel-webserver-module/src/apache/ApacheHooks.cpp +++ b/instrumentation/otel-webserver-module/src/apache/ApacheHooks.cpp @@ -634,7 +634,7 @@ int ApacheHooks::appd_hook_log_transaction_end(request_rec* r) if (appd_requestHasErrors(r)) { std::ostringstream oss; - oss << "HTTP ERROR CODE:" << r->status; + oss << r->status; res = wsAgent.endRequest(reqHandle, oss.str().c_str()); } else diff --git a/instrumentation/otel-webserver-module/src/core/api/RequestProcessingEngine.cpp b/instrumentation/otel-webserver-module/src/core/api/RequestProcessingEngine.cpp index f0097610a..0807e0659 100644 --- a/instrumentation/otel-webserver-module/src/core/api/RequestProcessingEngine.cpp +++ b/instrumentation/otel-webserver-module/src/core/api/RequestProcessingEngine.cpp @@ -19,17 +19,22 @@ #include "api/ApiUtils.h" #include "api/Payload.h" #include "sdkwrapper/SdkWrapper.h" +#include "sdkwrapper/IScopedSpan.h" +#include "sdkwrapper/SdkConstants.h" #include #include #include #include #include #include +#include namespace appd { namespace core { +using namespace sdkwrapper; + RequestProcessingEngine::RequestProcessingEngine() : mLogger(getLogger(std::string(LogContext::AGENT) + ".RequestProcessingEngine")) { @@ -99,10 +104,33 @@ APPD_SDK_STATUS_CODE RequestProcessingEngine::endRequest( // check for error and set attribute in the scopedSpan. if (error) { - rootSpan->SetStatus(sdkwrapper::StatusCode::Error, error); - LOG4CXX_TRACE(mLogger, "Setting status as error[" << error <<"] on root Span"); + std::string errorStatus; + std::stringstream strValue; + unsigned int errorValue; + + strValue << error; + strValue >> errorValue; + + strValue << kHttpErrorCode + error; // This is status message eg: HTTP ERROR CODE:403 + strValue >> errorStatus; + + if (errorValue >= HTTP_ERROR_1XX && errorValue < HTTP_ERROR_4XX ) { + rootSpan->SetStatus(StatusCode::Unset); + } + else if (errorValue >= HTTP_ERROR_4XX && errorValue < HTTP_ERROR_5XX ) { + if (rootSpan->GetSpanKind() == SpanKind::SERVER) + rootSpan->SetStatus(StatusCode::Unset); + else + rootSpan->SetStatus(StatusCode::Error, errorStatus); + + } else { + rootSpan->SetStatus(StatusCode::Error, errorStatus); + } + + LOG4CXX_TRACE(mLogger, "Setting status as error[" << errorStatus <<"] on root Span"); + } else { - rootSpan->SetStatus(sdkwrapper::StatusCode::Ok); + rootSpan->SetStatus(StatusCode::Ok); } LOG4CXX_TRACE(mLogger, "Ending root span with id: " << rootSpan.get()); @@ -137,7 +165,7 @@ APPD_SDK_STATUS_CODE RequestProcessingEngine::startInteraction( // TODO : confirm and update name later std::string spanName = payload->moduleName + "_" + payload->phaseName; keyValueMap["interactionType"] = "EXIT_CALL"; - auto interactionSpan = m_sdkWrapper->CreateSpan(spanName, sdkwrapper::SpanKind::CLIENT, keyValueMap); + auto interactionSpan = m_sdkWrapper->CreateSpan(spanName, SpanKind::CLIENT, keyValueMap); LOG4CXX_TRACE(mLogger, "Client Span started with SpanName: " << spanName << " Span Id: " << interactionSpan.get()); m_sdkWrapper->PopulatePropagationHeaders(propagationHeaders); @@ -177,11 +205,22 @@ APPD_SDK_API APPD_SDK_STATUS_CODE RequestProcessingEngine::endInteraction( // If errorCode is 0 or errMsg is empty, there is no error. bool isError = payload->errorCode != 0 && !payload->errorMsg.empty(); if (isError) { - interactionSpan->SetStatus(sdkwrapper::StatusCode::Error, payload->errorMsg); + if (payload->errorCode >= HTTP_ERROR_1XX && payload->errorCode < HTTP_ERROR_4XX ) { + interactionSpan->SetStatus(StatusCode::Unset); + } + else if (payload->errorCode >= HTTP_ERROR_4XX && payload->errorCode < HTTP_ERROR_5XX ) { + if (interactionSpan->GetSpanKind() == SpanKind::SERVER) + interactionSpan->SetStatus(StatusCode::Unset); + else + interactionSpan->SetStatus(StatusCode::Error, payload->errorMsg); + + } else { + interactionSpan->SetStatus(StatusCode::Error, payload->errorMsg); + } interactionSpan->AddAttribute("error_code", payload->errorCode); LOG4CXX_TRACE(mLogger, "Span updated with error Code: " << payload->errorCode); } else { - interactionSpan->SetStatus(sdkwrapper::StatusCode::Ok); + interactionSpan->SetStatus(StatusCode::Ok); } if (!payload->backendName.empty()) { diff --git a/instrumentation/otel-webserver-module/src/core/sdkwrapper/ScopedSpan.cpp b/instrumentation/otel-webserver-module/src/core/sdkwrapper/ScopedSpan.cpp index d9d001ab3..d697dd09b 100644 --- a/instrumentation/otel-webserver-module/src/core/sdkwrapper/ScopedSpan.cpp +++ b/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; } void ScopedSpan::End() @@ -68,6 +69,16 @@ void ScopedSpan::SetStatus(const StatusCode status, const std::string& desc) mSpan->SetStatus(otelStatus, desc); } +SpanKind ScopedSpan::GetSpanKind() +{ + if (mSpanKind == trace::SpanKind::kServer) + return SpanKind::SERVER; + else if (mSpanKind == trace::SpanKind::kClient) + return SpanKind::CLIENT; + else + return SpanKind::INTERNAL; +} + } //sdkwrapper } //core } //appd diff --git a/instrumentation/otel-webserver-module/src/core/sdkwrapper/ServerSpan.cpp b/instrumentation/otel-webserver-module/src/core/sdkwrapper/ServerSpan.cpp index 8e8112e45..240e7615c 100644 --- a/instrumentation/otel-webserver-module/src/core/sdkwrapper/ServerSpan.cpp +++ b/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); } +SpanKind ServerSpan::GetSpanKind() +{ + return SpanKind::SERVER; +} + + } //sdkwrapper } //core } //appd