chore(deps): Update github.com/gwos/tcg/sdk from v8.7.2 to v8.8.0 #15947

ymkins · 2024-09-27T17:07:30Z

Summary

[outputs.groundwork] update SDK

TCG SDK that provides client used for sending data to Groundwork Monitoring has updated.
Modern version got fixes and features. Internal logger was changed to slog for simplify integrations.

PR provides telegraf.Logger adapter for slog.Logger used in SDK. It allows to output SDK messages with Telegraf logger.

Also PR provides improvement to filter out non-UTF symbols in payloads.

Checklist

No AI generated code was used in this PR

Related issues

resolves #15945

ymkins · 2024-09-27T17:27:45Z

$ make lint plugins/outputs/groundwork/
golangci-lint run
plugins/outputs/groundwork/slog/slog.go:6:2  depguard  import 'log/slog' is not allowed from list 'main': Use injected telegraf.Logger instead
1 issues:
* depguard: 1
make: *** [Makefile:191: lint] Error 1

This package implements telegraf.Logger adapter for slog.Logger used in library. It's used to output library messages with Telegraf logger.

…nOutput

srebhan

Thanks a lot for you work @ymkins! I think you should tailor the implementation of the adapter to the concrete use-case here. Furthermore, I'm not convinced that logging library-internal messages with their log-level is the right approach. I would log all library-internal messages as level Trace to allow used to silence them as usually those should not be interesting for anyone...

plugins/outputs/groundwork/groundwork_test.go

plugins/outputs/groundwork/groundwork.go

plugins/outputs/groundwork/groundwork_test.go

plugins/outputs/groundwork/slog/slog.go

srebhan · 2024-09-30T12:40:59Z

plugins/outputs/groundwork/slog/slog.go

+	attrs := make([]slog.Attr, 0, 2+len(h.attrs)+r.NumAttrs())
+	attrs = append(attrs,
+		slog.String(h.MessageFieldName, r.Message),
+		slog.String(h.GroupsFieldName, strings.Join(h.groups, ",")),
+	)
+	for _, attr := range h.attrs {
+		if v, ok := attr.Value.Any().(json.RawMessage); ok {
+			attrs = append(attrs, slog.String(attr.Key, string(v)))
+			continue
+		}
+		attrs = append(attrs, attr)
+	}
+	r.Attrs(func(attr slog.Attr) bool {
+		if v, ok := attr.Value.Any().(json.RawMessage); ok {
+			attrs = append(attrs, slog.String(attr.Key, string(v)))
+			return true
+		}
+		attrs = append(attrs, attr)
+		return true
+	})


Why do you need to construct an array of attributes instead of directly constructing the message for logging?

Here we just preparing log data: adjusting datatype for json.RawMessage for better formatting. Then we letting actual logger make his work.
In output we getting well-structured message.

Yeah I was wondering why you don't directly format it into []string instead of keeping them as []slog.Attr...

There are non-string Attrs. Convert them here to string, then actual writer will convert it to bytes.. does it make sense? Onetime conversion in writer should be better.

plugins/outputs/groundwork/slog/slog.go

Update the implementation of the adapter for address review comments

srebhan

Thanks for the update @ymkins! Some more comments, with the most sever being the request to not work-around logging in tests.

srebhan · 2024-10-01T10:38:34Z

plugins/outputs/groundwork/groundwork_test.go

+	testLogger := func() telegraf.Logger {
+		if err := logger.SetupLogging(&logger.Config{Debug: true}); err != nil {
+			return nil
+		}
+		l := new(testutil.Logger)
+		log.Logger = NewLogger(l).WithGroup("tcg.sdk")
+		return l
+	}()
+


This should not be necessary! Please make sure the tests pass without this! Using testutil.Logger should be sufficient as otherwise there is something fishy going on... Same for the instances below.

srebhan · 2024-10-01T10:40:23Z

plugins/outputs/groundwork/log_adapter.go

+// NewLogger creates telegraf.Logger adapter for slog.Logger
+func NewLogger(l telegraf.Logger) *slog.Logger {
+	return slog.New(&TlgHandler{Log: l})
+}
+
+// TlgHandler translates slog.Record into telegraf.Logger call
+// inspired by https://github.com/golang/example/blob/master/slog-handler-guide/README.md
+type TlgHandler struct {


Please don't export these things! Maybe even kill off the NewLogger function completely as it only wraps a single line. However, I see the value in keeping log/slog only in here...

yes, there is the value to keep log/slog only in here
and done TlgHandler => tlgHandler

srebhan · 2024-10-01T10:44:04Z

plugins/outputs/groundwork/log_adapter.go

+	for _, attr := range h.attrs {
+		if v, ok := attr.Value.Any().(json.RawMessage); ok {
+			attrs = append(attrs, slog.String(attr.Key, string(v)))
+			continue
+		}
+		attrs = append(attrs, attr)
+	}


Couldn't this conversion be already done in WithAttrs? This would save a few cycles and simplify code.

telegraf-tiger · 2024-10-03T21:09:32Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_arm64.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz

telegraf-tiger bot added the chore label Sep 27, 2024

Pavlo Sumkin and others added 2 commits September 27, 2024 21:34

chore(deps): update github.com/gwos/tcg/sdk to v8.8.0

66e9610

chore(outputs.groundwork): Use only valid UTF-8 strings for LastPlugi…

f94db3c

…nOutput

ymkins force-pushed the feat/groundwork-output branch from 5058565 to f94db3c Compare September 27, 2024 19:35

Merge branch 'master' into feat/groundwork-output

a72e514

srebhan reviewed Sep 30, 2024

View reviewed changes

srebhan self-assigned this Sep 30, 2024

chore(outputs.groundwork): Simplify log-adapter

92ce908

Update the implementation of the adapter for address review comments

srebhan reviewed Oct 1, 2024

View reviewed changes

chore(outputs.groundwork): Simplify log-adapter: part2

cd108cf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(deps): Update github.com/gwos/tcg/sdk from v8.7.2 to v8.8.0 #15947

chore(deps): Update github.com/gwos/tcg/sdk from v8.7.2 to v8.8.0 #15947

ymkins commented Sep 27, 2024

ymkins commented Sep 27, 2024

srebhan left a comment

srebhan Sep 30, 2024

ymkins Sep 30, 2024

srebhan Oct 1, 2024

ymkins Oct 3, 2024

srebhan left a comment

srebhan Oct 1, 2024

ymkins Oct 3, 2024

srebhan Oct 1, 2024

ymkins Oct 3, 2024

srebhan Oct 1, 2024

ymkins Oct 3, 2024

telegraf-tiger bot commented Oct 3, 2024

Artifact URLs

chore(deps): Update github.com/gwos/tcg/sdk from v8.7.2 to v8.8.0 #15947

Are you sure you want to change the base?

chore(deps): Update github.com/gwos/tcg/sdk from v8.7.2 to v8.8.0 #15947

Conversation

ymkins commented Sep 27, 2024

Summary

Checklist

Related issues

ymkins commented Sep 27, 2024

srebhan 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

Choose a reason for hiding this comment

srebhan 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telegraf-tiger bot commented Oct 3, 2024

Artifact URLs