You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/11/05 04:57:40 UTC
[GitHub] [trafficserver] maskit opened a new pull request #8495: Add support for OpenTracing
maskit opened a new pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495
This adds very basic support for [OpenTracing](https://opentracing.io).
- Build option to use OpenTracing
- Settings to enable tracing
- Code to initialize libraries
- Generic interface for tracing
- Code to trace transactions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on a change in pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
sudheerv commented on a change in pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#discussion_r779851680
##########
File path: include/tscore/Tracing.h
##########
@@ -0,0 +1,119 @@
+/** @file
+
+ ATS Tracing API
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you 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
+
+#include <atomic>
+#include <memory>
+#include <string_view>
+#include "tscore/ink_assert.h"
+#include "tscore/ink_config.h"
+
+#if TS_USE_OPENTRACING
+#include "tscore/Tracing_opentracing.h"
+#else
+using TRACER = int;
+inline void
+tracing_tag(TRACER &out, std::string_view name, std::string_view value)
+{
+ ink_assert(!"No tracing library is available");
+}
+
+inline void
+tracing_tag(TRACER &out, std::string_view name, int value)
+{
+ ink_assert(!"No tracing library is available");
+}
+
+inline void
+tracing_log(TRACER &out, std::string_view category, std::string_view tag)
+{
+ ink_assert(!"No tracing library is available");
+}
+
+inline void
+tracing_log(TRACER &out, std::string_view category, int value)
+{
+ ink_assert(!"No tracing library is available");
+}
+
+inline TRACER *
+tracing_new(const char *name)
+{
+ return nullptr;
+}
+
+inline void
+tracing_delete(TRACER *tracer)
+{
+}
+#endif
+
+class Tracing
+{
+public:
+ void enable(int value = 1);
+ void disable();
+
+ bool is_enabled();
+
+ TRACER *make_tracer(const char *name);
+ void delete_tracer(TRACER *tracer);
+
+private:
+ std::atomic<int> _enabled;
+};
+
+inline bool
+Tracing::is_enabled()
+{
+ return _enabled > 0;
+}
+
+inline TRACER *
+Tracing::make_tracer(const char *name)
+{
+ return tracing_new(name);
+}
+
+inline void
+Tracing::delete_tracer(TRACER *tracer)
+{
+ tracing_delete(tracer);
+}
+
+extern std::unique_ptr<Tracing> tracing;
+
+#define TRACE_TAG(out, category, message) \
Review comment:
I think we should have TS API to allow plugins to leverage/add tags too. Using Tracing just within ATS core may not have much value as the Tracing is primarily meant to solve the distributed system use case (trace request as it travels through micro services).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#issuecomment-961656609
> i have been using opentracing in a different context. i.e. in a plugin to allow http requests passing through ATS to participate in end-to-end, distributed tracing with jaeger in microservice environment.
I think that is how OpenTracing is used by nginx and envoy, and that would be also nice to support. The library detection part should be useful for that case too if we have a plugin like that in our source tree.
> If we are to add tracing support inside ATS, should we consider using opentelemetry instead?
Adding support for OpenTelemetry is fine, but I'm not sure if we should only support OpenTelemetry. I defined a generic interface so that we can support different tracing systems without adding many `#ifdef` here and there. OpenTelemetry library for C++ made version 1.0 just a few months ago.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#issuecomment-1007170543
Closing this PR because there's no demand for support for a deprecated library and an abstraction layer to support multiple tracing systems. The consensus here was that we should only support OpenTelemetry without abstraction layers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
sudheerv commented on pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#issuecomment-1006847291
+1 on OpenTelemetry (OTEL). OpenTracing and OpenCensus have merged into OTEL and are on path to deprecation, so we may not gain much benefit in adding support for OpenTracing at this point. OTEL is also backward compatible with OpenTracing (and OpenCensus) as well.
Coincidentally, we are adding support for Distributed Tracing internally and are adopting OTEL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shukitchan commented on pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
shukitchan commented on pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#issuecomment-961644371
i have been using opentracing in a different context. i.e. in a plugin to allow http requests passing through ATS to participate in end-to-end, distributed tracing with jaeger in microservice environment.
And I have since moved on to investigate using opentelemetry instead.
If we are to add tracing support inside ATS, should we consider using opentelemetry instead?
https://opensource.googleblog.com/2019/05/opentelemetry-merger-of-opencensus-and.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#issuecomment-961656609
> i have been using opentracing in a different context. i.e. in a plugin to allow http requests passing through ATS to participate in end-to-end, distributed tracing with jaeger in microservice environment.
I think that is how OpenTracing is used by nginx and envoy, and that would be also nice to support. The library detection part should be useful for that case too if we have a plugin like that in our source tree.
> If we are to add tracing support inside ATS, should we consider using opentelemetry instead?
Adding support for OpenTelemetry is fine, but I'm not sure if we should only support OpenTelemetry. I defined a generic interface so that we can support different tracing systems without adding many `#ifdef` here and there. OpenTelemetry library for C++ made version 1.0 just a few months ago.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#discussion_r745189488
##########
File path: include/tscore/Tracing.h
##########
@@ -0,0 +1,119 @@
+/** @file
+
+ ATS Tracing API
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you 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
+
+#include <atomic>
+#include <memory>
+#include <string_view>
+#include "tscore/ink_assert.h"
+#include "tscore/ink_config.h"
+
+#if TS_USE_OPENTRACING
+#include "tscore/Tracing_opentracing.h"
+#else
+using TRACER = int;
+inline void
+tracing_tag(TRACER &out, std::string_view name, std::string_view value)
+{
+ ink_assert(!"No tracng library is available");
Review comment:
"tracng"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shukitchan commented on pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
shukitchan commented on pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#issuecomment-1007052327
I don't think there is any other well-maintained alternative to opentelemetry. Perhaps we should close this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shukitchan commented on pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
shukitchan commented on pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#issuecomment-961644371
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#issuecomment-980523699
The slides I used for the Summit last week is on the wiki.
https://cwiki.apache.org/confluence/display/TS/Presentations+-+2021
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shukitchan commented on a change in pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
shukitchan commented on a change in pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#discussion_r776859065
##########
File path: include/tscore/Tracing_opentracing.h
##########
@@ -0,0 +1,94 @@
+/** @file
+
+ ATS Tracing API implementation (OpenTracing)
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you 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
+
+#include <opentracing/dynamic_load.h>
+#include <opentracing/tracer.h>
+#include <opentracing/span.h>
+#include <string_view>
+#include <vector>
+#include "ink_thread.h"
+#include "Diags.h"
+
+using TRACER = opentracing::Span;
+
+extern opentracing::expected<opentracing::DynamicTracingLibraryHandle, std::error_code> ot_lib;
+extern std::string ot_config;
+extern std::mutex tracers_mutex;
+extern std::vector<std::shared_ptr<opentracing::Tracer>> tracers;
+extern ink_thread_key thread_specific_tracer_key;
+extern std::string ot_config;
+extern opentracing::expected<opentracing::DynamicTracingLibraryHandle, std::error_code> ot_lib;
Review comment:
are these duplicate?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit closed pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
maskit closed pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shukitchan commented on pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
shukitchan commented on pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#issuecomment-962027053
I just have the concern that since the announcement of opentracing and opencensus merging to form opentelemetry in 2019, there are not a lot of meaningful updates on the opentracing github repos. (e,.g. https://github.com/opentracing/opentracing-cpp) . So I wonder if the library is still being maintained or in life support mode. And therefore whether we should still be using it or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#issuecomment-963693229
@shukitchan Can you take a look at this? Might want to review after @maskit gives a talk on this at the Fall Summit next week.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#issuecomment-1007170543
Closing this PR because there's no demand for support for a deprecated library and an abstraction layer to support multiple tracing systems. The consensus here was that we should only support OpenTelemetry without abstraction layers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#issuecomment-963741105
> there are not a lot of meaningful updates on the opentracing github repos.
Right, but I wouldn't expect a lot of changes on opentracing-cpp repo, because it just provides a bridge for implementations. At least an actual cpp implementation ([jaeger](https://github.com/jaegertracing/jaeger-client-cpp/releases)) is still maintained, although their documentation recommends using OpenTelemetry for new applications.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shukitchan commented on pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
shukitchan commented on pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#issuecomment-961644371
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#issuecomment-961656609
> i have been using opentracing in a different context. i.e. in a plugin to allow http requests passing through ATS to participate in end-to-end, distributed tracing with jaeger in microservice environment.
I think that is how OpenTracing is used by nginx and envoy, and that would be also nice to support. The library detection part should be useful for that case too if we have a plugin like that in our source tree.
> If we are to add tracing support inside ATS, should we consider using opentelemetry instead?
Adding support for OpenTelemetry is fine, but I'm not sure if we should only support OpenTelemetry. I defined a generic interface so that we can support different tracing systems without adding many `#ifdef` here and there. OpenTelemetry library for C++ made version 1.0 just a few months ago.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#issuecomment-1007044257
I do think it's nice to add support for OpenTelemetry and the distributed system use case in the future, but those are out of my scope. I probably don't work on those until I need those, although I'll be happy to review PRs for those. It should not be too difficult to add those supports on top of this change.
I'm going to add documentation unless you think we should not support OpenTracing, but If you think OpenTelemetry will be one and only distributed tracing system and we won't need a complicated abstraction layer, I'd close this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
sudheerv commented on pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495#issuecomment-1007110755
> I do think it's nice to add support for OpenTelemetry and the distributed system use case in the future, but those are out of my scope. I probably don't work on those until I need those, although I'll be happy to review PRs for those. It should not be too difficult to add those supports on top of this change.
>
> I'm going to add documentation unless you think we should not support OpenTracing, but
>>> If you think OpenTelemetry will be one and only distributed tracing system and we won't need a complicated abstraction layer, I'd close this PR.
Yeah, this definitely seems to be the current direction for distributed tracing. OpenTracing and OpenCensus are both deprecated and are on path to EOL. Both the communities have joined hands and are focusing on OpenTelemetry which ensures backward compatibility with both and is the one where there is quite a bit of interest and adoption across the industry. Even the most popular Jaeger deprecated their own clients and are recommending to switch to OTEL. As such, we may not want to support OpenTracing at this stage. I’m not strongly opposed to it but I’m not too sure if there’s a lot of value in creating an abstraction layer given that OTEL is the only potential standard for the near future at least.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit closed pull request #8495: Add support for OpenTracing
Posted by GitBox <gi...@apache.org>.
maskit closed pull request #8495:
URL: https://github.com/apache/trafficserver/pull/8495
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org