moves clog into clues #56

ryanfkeepers · 2024-10-14T22:11:30Z

Transferring Clog into Clues as a subpackage will solve:

multi-package updates (clues -> clog -> something with both clog and clues)
better preparation for OTEL support integration into clues and clog.
sets up future extensions like metrics tracking via clues.

clog/README.md

clog/builder.go

ashmrtn · 2024-10-17T21:29:35Z

clog/logger.go

+//
+// Unless you call this before initialization.  Then it'll panic.  We do want you to init
+// the logger first, else you'll potentially lose these logs due different buffers.
+func Singleton() *builder {


I'm not really a fan of having something like this. If we run into the situation described, it's probably a design issue that should be addressed. I'm concerned something like this will allow more design issues to slip through because they allow kind of papering over the root problem

I'm back and forth on it. I do agree that not providing this method makes a good forcing function to resolve cases that would sloppily use it. I'm also aware that every now and then devs will hit a corner case where this sort of accessibility makes life much less painful or arduous. I'm not in a rush to do so, but we can take it out if there are strong feelings against the design. That's a two-way-door.

I think it would be best to remove something like this until we have more insight into what other SDKs clues will be used with (e.x. this wouldn't work if we needed to use it with temporal)

Temporal already has logging hooks built in, so there's no concerns with that. But, sure, I can remove it.

ashmrtn · 2024-10-17T21:31:46Z

clog/settings.go

+}
+
+// Returns the default location for log file storage.
+func defaultLogLocation() string {


setting the default location and using a global env var to set the location seem problematic. Now two programs that consume this package can't run on the same machine with the defaults, and it's not clear what could happen if people do run with the defaults

Good catch. Can we put a pin in this and follow up later? Current presumption is that live runs will feed to Stderr by default, not to log files, and that some background receiver will handle serialization of stderr logs to storage.

I'm still rather uncomfortable with the assumptions being made about setup of the logging component. This is technically an open-source library and I'm sure we'd like to see usage in our own systems going forward. However, the logging setup has hidden configurations that will lead to difficult to debug issues if not noticed while also making some pretty heavy assumptions about usage in general (e.x. assume sent to stderr which isn't really the standard when it comes to outputting log files AFAIK)

If the goal is adoption and usage then, given that this is basically a core feature that everything is going to rely on, the implementation should be robust enough that folks can easily do what they need and are unlikely to hit unexpected issues (this kind of gets back to "how soon is later"?)

I have no disagreement with your assesment. But is it really a blocker to punt on the issue for this PR? The code already matches this in the existing clog repo, so we're not avoiding the issue at this moment. And I will follow up on it. But this PR is supposed to be a copy-paste, not a fixup of the repo.

clog/logger.go

ashmrtn · 2024-10-17T21:34:33Z

clog/builder_test.go

+
+	for _, tt := range table {
+		suite.T().Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.expected, getValue(tt.value))


I'm not actually sure if this will just end up testing the interface test case a bunch of time since that's the type in the struct or if it will be testing the underlying types

we test both casses with this spread. I'll add some other cases for coverage, just to have that clear.

I think maybe we're on slightly different trains of thought here? My concern is whether declaring the test table struct to have value any for the input will result in always testing the interface segment of the code under test since technically any is an alias to interface{}. If that is the way the code behaves, then adding additional test cases to this test won't change anything

I think to figure out how this is actually run, you'd need to add temporary print statements to the code

I think to figure out how this is actually run, you'd need to add temporary print statements to the code

Heh, no, I grok. And that's exactly what I did. All the other changes were because I figured I was already in the code and wanted to touch things up a bit.

ashmrtn · 2024-10-17T21:37:20Z

sets up future extensions like metrics tracking via clues.

Overall, I think we need to be careful about scope creep. It's nice to have one thing that does everything and the kitchen sink, but the thing needs to work really well for that to really be a good situation. The larger the scope, the more likely it is for some areas or others to not be as high quality as we'd like

With a package like this, there's also the chance that we end up hooking together things in such a way that we're getting too much data back. Having too much data can be just as crippling as not having enough. If we really want to add metrics, we need to be very careful about doing so

ryanfkeepers · 2024-10-18T18:55:17Z

If we really want to add metrics, we need to be very careful about doing so

Ah, metrics is intended to be a sort of isolated sub-package. I don't plan on a coupled implementatio with clues or clog as it stands. Expectation is for it to be loosely coupled in the way those two packages are. The primary connection being that the metrics subpackage would extract otel information from a clues context, the same way clog does in the next PR on this branch. Otherwise the goal is for it to feel separate and optional.

Transferring Clog into Clues as a subpackage will solve: 1. multi-package updates (clues -> clog -> something with both clog and clues) 2. better preparation for OTEL support integration into clues and clog. 3. sets up future extensions like metrics tracking via clues.

ashmrtn

generally LGTM, unresolved comments still need addressed though

ashmrtn · 2024-11-04T21:50:52Z

clog/builder.go

+	return b
+}
+
+// Label adds all of the appended labels to the error.


Suggested change

// Label adds all of the appended labels to the error.

// Label adds all of the appended labels to the log.

fixed in another PR

ashmrtn · 2024-11-04T23:48:01Z

clog/builder_test.go

+
+	for _, tt := range table {
+		suite.T().Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.expected, getValue(tt.value))


I think maybe we're on slightly different trains of thought here? My concern is whether declaring the test table struct to have value any for the input will result in always testing the interface segment of the code under test since technically any is an alias to interface{}. If that is the way the code behaves, then adding additional test cases to this test won't change anything

I think to figure out how this is actually run, you'd need to add temporary print statements to the code

ryanfkeepers added the enhancement New feature or request label Oct 14, 2024

ryanfkeepers requested review from ashmrtn, pandeyabs, meain and HiteshRepo October 14, 2024 22:11

ryanfkeepers self-assigned this Oct 14, 2024

ryanfkeepers force-pushed the add-clog branch from 53afe8a to aa9d90d Compare October 14, 2024 22:18

ashmrtn reviewed Oct 17, 2024

View reviewed changes

ryanfkeepers force-pushed the add-clog branch from e01b3d8 to 09d5882 Compare October 31, 2024 20:38

ryanfkeepers force-pushed the add-clog branch from 09d5882 to 92bca4f Compare October 31, 2024 20:38

ryanfkeepers requested a review from m-terel October 31, 2024 22:10

ashmrtn reviewed Nov 4, 2024

View reviewed changes

ashmrtn approved these changes Nov 4, 2024

View reviewed changes

ryanfkeepers merged commit 5fd5083 into main Nov 4, 2024
1 check passed

ryanfkeepers deleted the add-clog branch November 4, 2024 23:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

moves clog into clues #56

moves clog into clues #56

ryanfkeepers commented Oct 14, 2024

ashmrtn Oct 17, 2024

ryanfkeepers Oct 18, 2024

ashmrtn Nov 4, 2024

ryanfkeepers Nov 4, 2024

ryanfkeepers Nov 4, 2024

ashmrtn Oct 17, 2024

ryanfkeepers Oct 18, 2024

ashmrtn Nov 4, 2024 •

edited

Loading

ryanfkeepers Nov 4, 2024

ryanfkeepers Nov 4, 2024

ashmrtn Oct 17, 2024

ryanfkeepers Nov 4, 2024

ryanfkeepers Nov 4, 2024

ashmrtn Nov 4, 2024

ryanfkeepers Nov 4, 2024

ashmrtn commented Oct 17, 2024

ryanfkeepers commented Oct 18, 2024

ashmrtn left a comment

ashmrtn Nov 4, 2024

ryanfkeepers Nov 4, 2024

ashmrtn Nov 4, 2024

	// Label adds all of the appended labels to the error.
	// Label adds all of the appended labels to the log.

moves clog into clues #56

moves clog into clues #56

Conversation

ryanfkeepers commented Oct 14, 2024

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

ashmrtn Nov 4, 2024 • 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

ashmrtn commented Oct 17, 2024

ryanfkeepers commented Oct 18, 2024

ashmrtn left a 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

ashmrtn Nov 4, 2024 •

edited

Loading