Remove TSDebug() and TSIsDebugTagSet() TS API functions. #10390

ywkaras · 2023-09-08T20:07:09Z

Replace with Dbg() and DbgCtl::on(). This required extensive changes to the ESI plugin unit tests.

Doc changes were also necessary.

The redirect_1 example plugin is changed to illustrate how to implement debug trace output in a plugin that can compile and run with either these ATS10 changes or for older versions of ATS.

ywkaras · 2023-09-13T18:07:49Z

@cmcfarlen this is passing CI now.

ywkaras · 2023-09-19T02:02:50Z

@cmcfarlen It's successfully rebased.

cmcfarlen

This is mostly mechanical. I am concerned that the esi plugin will generate a lot of tags for the tag registry. Can you explain how that works?

cmcfarlen · 2023-09-19T02:08:47Z

doc/developer-guide/api/functions/TSTrafficServerVersionGet.en.rst


    #define PLUGIN_NAME "hello_world"

+    DbgCtl dbg_ctl{PLUGIN_NAME};


Do you want to establish the convention of putting these in anonymous namespaces here?

I think it's not that simple. If I add an anonymous namespace, check_ts_version() probably belongs in it also. But, if I make such changes consistently, in all the files I'm touching, it will likely double the size of this PR. I get a lot of surprising push back in reviews on this project. For example, push back that seems to dispute the value of RAII. Which makes me feel like adding any sort of cleanup risks adding further delay to merging PRs.

I just mean in this example, since folks tend to copy pasta examples. If we want to prefer anonymous namespaces, having that in the example will help with proliferation.

cmcfarlen · 2023-09-19T02:13:28Z

example/plugins/c-api/protocol/Protocol.h

+{
+extern DbgCtl dbg_ctl;
+}
+using namespace protocol_ns;


I know this is just an example, but putting this using namespace in a header might lead to user astonishment.

I see your point, but what do you see as the better alternative?

TS developers must steel themselves to endure astonishment, and not in a good way.

cmcfarlen · 2023-09-19T02:21:07Z

plugins/cache_promote/policy.h


+extern DbgCtl cache_promote_dbg_ctl;
+
+#define DBG(...) Dbg(cache_promote_dbg_ctl, __VA_ARGS__)


Why is this plugin converted with a macro rather than Dbg?

I think I did this before I thought of the other approach with using namespace.

cmcfarlen · 2023-09-19T02:27:32Z

plugins/esi/esi.cc

      input_type(DATA_TYPE_RAW_ESI),
      packed_node_list(""),
      gzipped_data(""),
+      dbg_ctl{createDebugTag(DEBUG_TAG, contptr)},


Won't this insert a lot of tags into the registry? How often is ContData instantiated?

If you create multiple DbgCtl instances for the same tag, they will all point to the same entry in the registry map. In any case, I think each instance of this continuation class will have a unique debug tag. So there's a net increase, per class instance, of 8 bytes for the pointer in the DbgCtl, plus the overhead of an additional std::map entry.

It looks like a new ContData instance is created per transaction that this plugin handles. Since the tag is created using the address of the contptr instance, I feel like there is a good chance that there will be a lot of additional tags due to this.

But my changes don't increase the number of instances of ContData. I see the issue, but what can I do about it?

It seems like these tags are generated to add correlation context to the log statements and not as a way to filter them with the tagging mechanism. One idea would be to move this context from the tag name into the log message and have a fixed number of dbgctl tags.

But does that really belong in this PR? The change you suggest would reduce memory use in the existing code. It can't clearly be claimed it's addressing an issue that I created.

Correct me if I am wrong, but as I understand it, any tags created by DbgCtl instances will only be cleaned up when the Registry destructor is called. The DbgCtl destructor only decrements the reference counter. Therefore, if some code (i.e. this very esi plugin), creates a new DbgCtl instance per ATS transaction, the size of the tag registry will grow unbounded.

OK, valid point.

Replace with Dbg() and DbgCtl::on(). This required extensive changes to the ESI plugin unit tests. Doc changes were also necessary. The redirect_1 example plugin is changed to illustrate how to implement debug trace output in a plugin that can compile and run with either these ATS10 changes or for older versions of ATS.

_No_tag_dummy is changed to a static member function that returns a function-local static variable. Also, to avoid changing gold files with debug output, diags.show_location is disabled for the Au tests that test for debug output using gold files.

cmcfarlen

Nicely done. Thank you!

ywkaras force-pushed the KillTSDebug branch 11 times, most recently from 83bd148 to cfa30c9 Compare September 11, 2023 19:37

bryancall assigned ywkaras Sep 11, 2023

bryancall added the Debug Support for system debugging label Sep 11, 2023

bryancall added this to the 10.0.0 milestone Sep 11, 2023

bryancall requested a review from cmcfarlen September 11, 2023 22:13

ywkaras force-pushed the KillTSDebug branch 6 times, most recently from cc3bd63 to 1fd211d Compare September 13, 2023 17:39

ywkaras force-pushed the KillTSDebug branch from 1fd211d to 14116f4 Compare September 19, 2023 00:17

cmcfarlen reviewed Sep 19, 2023

View reviewed changes

ywkaras added 3 commits September 20, 2023 16:32

Make necessary change to ESI plugin unit test CMakeLists.txt.

7f00b42

ywkaras force-pushed the KillTSDebug branch from 14116f4 to c7b83b6 Compare September 21, 2023 00:05

Reviews changes.

39535d7

ywkaras force-pushed the KillTSDebug branch from c7b83b6 to 39535d7 Compare September 21, 2023 00:50

cmcfarlen approved these changes Sep 22, 2023

View reviewed changes

ywkaras merged commit 87af7b4 into apache:master Sep 22, 2023

bneradt mentioned this pull request Sep 27, 2023

certifier.so: Convert to TSDbg #9809

Closed

zwoop added the Incompatible label Jul 9, 2025


		#define PLUGIN_NAME "hello_world"

		DbgCtl dbg_ctl{PLUGIN_NAME};


		extern DbgCtl cache_promote_dbg_ctl;

		#define DBG(...) Dbg(cache_promote_dbg_ctl, __VA_ARGS__)

Remove TSDebug() and TSIsDebugTagSet() TS API functions. #10390

Remove TSDebug() and TSIsDebugTagSet() TS API functions. #10390

Uh oh!

Conversation

ywkaras commented Sep 8, 2023

Uh oh!

ywkaras commented Sep 13, 2023

Uh oh!

ywkaras commented Sep 19, 2023

Uh oh!

cmcfarlen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmcfarlen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants