remove viper, fix envvars #120

tobert · 2023-01-10T14:06:34Z

This is a derivative / alternate to #117 that removes Viper in favor of manually reading envvars and a json config. It also addresses #109.

As mentioned in #83 this brings the binary size down by more than a megabyte compared to #117. Since the viper configs never worked and weren't clearly documented in the first place this seems reasonable to me.

Before (109-fix-envvar-settings branch):

atobey@usagi:~/src/otel-cli$ git checkout 109-fix-envvar-settings && go build && ls -l otel-cli
Switched to branch '109-fix-envvar-settings'
Your branch is up to date with 'origin/109-fix-envvar-settings'.
-rwxr-xr-x 1 atobey atobey 18043798 Jan  9 20:18 otel-cli

After (experiment-remove-viper branch):

atobey@usagi:~/src/otel-cli$ git checkout experiment-remove-viper && go build && ls -l otel-cli
Switched to branch 'experiment-remove-viper'
Your branch is up to date with 'origin/experiment-remove-viper'.
-rwxr-xr-x 1 atobey atobey 16871993 Jan  9 20:18 otel-cli

This never should have worked. Next commit will fix the span data checker.

Working on a test for #109 it became clear there were some faults in how checkSpanData worked. For one it wasn't validating that an expected 'is_sampled' was actually in the received span. It turns out that field was never available for comparison. That check was added. Also only the regex fields were being checked because of looping over that map so I moved it up to a global map and now loop over received fields and compare against instead of the other way around. This should be more exhaustive.

In order to test service name setting, this needs to come through in the tests. This patch does that. Refactored attribute flattening code to its own function.

This test should pass once the bug is fixed.

also test multiple attributes are handled correctly

viper seems to need these. the service name envvar works now. OTEL_CLI_ATTRIBUTES still does not work.

test fails in a more interesting way now always check your errors, kids

The one in pflags is buried deep so we need our own to make Viper do the right thing.

This isn't ideal since the type information is there somewhere and it could be converted cleanly. That said, I dug through the grpc and otel code trying to figure it out and didn't make much progress. Since this server isn't super serious, it should be ok to clean up with string manipulation and get all the tests to work as expected. This seems to work for now so proper type conversion can happen another day. Sorry.

By default viper will parse JSON in envvars but not the comma-separated k=v style that cobra/pflag does. This implements a hook to intercept those values and decode them manually, which makes OTEL_CLI_ATTRIBUTES finally work as advertised.

For now, let's make sure folks can build widely. Probably should make a decision soon about minimum Go compiler though.

We discussed this in #83. It seems to be less code overall, even before removing the dependency.

I'm not super sure this is the right move but feels like it follows policy.

Wrote a test to verify parsing fails on an invalid environment variable value. Because softFail uses log.Fatal, it includes a date stamp which is the right thing to do. To make testing easier, this called for a way to modify the output before comparing to the fixture data, thus I added Results.CliOutputRe which takes a regex and always deletes the match.

It's json-only now, and an example is added. I'm not thrilled about the ordering of Defaults -> CLI -> environment -> file but it works and is consistent.

In order to fix the config test, a "*" option needed to be added to the test harness so I did that. Cleaned up up README and other things after getting the test right, so there are a few odds & ends in this commit that are related.

edw-eqix · 2023-01-10T18:17:59Z

otlpserver/clievent.go

@@ -146,3 +146,22 @@ func NewCliEventFromSpanEvent(se *tracepb.Span_Event, span *tracepb.Span, ils *t

 	return e
 }
+
+// mapToKVString flattens attribute string maps into "k=v,k=v" strings.
+func mapToKVString(in map[string]string) string {


this generic function could be moved to helpers.go?

I think there's another version of the same function already in there, but private. We could make that public and de-duplicate easily enough. I'll opened #121 to follow up.

edw-eqix · 2023-01-10T18:35:34Z

otelcli/config.go

+			case string:
+				target.SetString(envVal)
+			case int:
+				intVal, err := strconv.ParseInt(envVal, 10, 64)


nit: add a comment that we want the widest possible int

It's only 64 bit to match the signature of the call to (reflect.Value).target.SetInt(x int64)

I tried 32 bit first when I was writing it but going 64 bit saves a type conversion :)

edw-eqix · 2023-01-10T19:05:41Z

main_test.go

+	// if an expected config string is set to "*" it will match anything
+	// and is effectively ignored
+	for k, v := range wantConf {
+		if v == "*" {
+			// set to same so cmd.Diff will ignore
+			wantConf[k] = gotConf[k]
+		}
+	}


not for this commit, but * representing .+ with additional checks below feels like a lot of magic. i opted not to file an issue because I don't have a strong sense of what would be better here, but perhaps this could be tracked as an improvement?

As we discussed elsewhere, I think if * becomes a problem the next step is to switch those strings to require a *regexp.Regexp. This would be powerful, and a bit sharper. Perhaps the first thing to explore is if some of those * use cases could be clearer and more precise, e.g. Expect.TraceId = "*" becomes

Expect.TraceId = regexp.MustCompile(`^[0-9a-fA-F]{32}$`)

I'm not sure it's worth it yet but it'd be cool to explore as the test suite grows. An issue would be cool, it's a good question and I want to get the test suite to a spot where we feel confident releasing automatically.

* add a failing test for span background --attrs Actually modifying an existing test to use the feature. * add missing func to support span background --attrs test still fails but fails less badly, next to fix the map[string]string translations * fix test data so test passes The test was getting the right data but failing becaues of a TODO in `otlpserver/server.go`. That TODO involves building a type switch and supporting at least a subset of otel types so rather than doing that right now, just have the test accept the format with the type in it. * is_sampled was never available in span data This never should have worked. Next commit will fix the span data checker. * rework checkSpanData for correctness Working on a test for #109 it became clear there were some faults in how checkSpanData worked. For one it wasn't validating that an expected 'is_sampled' was actually in the received span. It turns out that field was never available for comparison. That check was added. Also only the regex fields were being checked because of looping over that map so I moved it up to a global map and now loop over received fields and compare against instead of the other way around. This should be more exhaustive. * add failing test for #109 * update test The type tags leaking through in strings was fixed in #120 so it doesn't have to be in the fixture strings anymore.

tobert added 24 commits January 6, 2023 15:04

is_sampled was never available in span data

383c4f8

This never should have worked. Next commit will fix the span data checker.

add failing test for #109

11e5ade

plumb service attributes into CliEvent

13a081c

In order to test service name setting, this needs to come through in the tests. This patch does that. Refactored attribute flattening code to its own function.

make a failing test for service name

2cf2608

This test should pass once the bug is fixed.

make attr map flattening more ergonomic

12c3007

also test OTEL_RESOURCE_ATTRIBUTES

8e107e6

also test multiple attributes are handled correctly

Merge branch 'equinix-labs:main' into 109-fix-envvar-settings

67d923a

add mapstructure struct tags

105e63b

viper seems to need these. the service name envvar works now. OTEL_CLI_ATTRIBUTES still does not work.

check error on viper.Unmarshal 🤦

3f302fc

test fails in a more interesting way now always check your errors, kids

add a comment

6a28445

add parseCkvStringMap helper & test

27ab0fd

The one in pflags is buried deep so we need our own to make Viper do the right thing.

implement & use mapstructure decode hook

e07e9b2

By default viper will parse JSON in envvars but not the comma-separated k=v style that cobra/pflag does. This implements a hook to intercept those values and decode them manually, which makes OTEL_CLI_ATTRIBUTES finally work as advertised.

use strings.SplitN instead of Cut for older go

051b86c

For now, let's make sure folks can build widely. Probably should make a decision soon about minimum Go compiler though.

replace viper with custom environment loading

e80819b

We discussed this in #83. It seems to be less code overall, even before removing the dependency.

clean up TODOs, wrap errors

2bb9119

make env parse failures a soft failure

4adc540

I'm not super sure this is the right move but feels like it follows policy.

remove accidental comment

3652a4a

re-implment config file loading

0fa3768

It's json-only now, and an example is added. I'm not thrilled about the ordering of Defaults -> CLI -> environment -> file but it works and is consistent.

update README docs for configuration

70ca181

fix up config test, add needed infra for it

27b0791

In order to fix the config test, a "*" option needed to be added to the test harness so I did that. Cleaned up up README and other things after getting the test right, so there are a few odds & ends in this commit that are related.

correct comment

57d4acd

edw-eqix approved these changes Jan 10, 2023

View reviewed changes

tobert mentioned this pull request Jan 11, 2023

de-duplicate otlpserver.mapToKVString and otelcli.flattenStringMap #121

Closed

tobert merged commit 8eb37fb into equinix-labs:main Jan 11, 2023

tobert mentioned this pull request Jan 11, 2023

#109 fix envvar settings #117

Closed

tobert mentioned this pull request Feb 14, 2023

support standard OpenTelemetry envvars where appropriate #11

Closed

tobert deleted the experiment-remove-viper branch May 10, 2023 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove viper, fix envvars #120

remove viper, fix envvars #120

tobert commented Jan 10, 2023

edw-eqix Jan 10, 2023

tobert Jan 11, 2023

edw-eqix Jan 10, 2023

tobert Jan 11, 2023

edw-eqix Jan 10, 2023

tobert Jan 11, 2023 •

edited

Loading

remove viper, fix envvars #120

remove viper, fix envvars #120

Conversation

tobert commented Jan 10, 2023

edw-eqix Jan 10, 2023

Choose a reason for hiding this comment

tobert Jan 11, 2023

Choose a reason for hiding this comment

edw-eqix Jan 10, 2023

Choose a reason for hiding this comment

tobert Jan 11, 2023

Choose a reason for hiding this comment

edw-eqix Jan 10, 2023

Choose a reason for hiding this comment

tobert Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

tobert Jan 11, 2023 •

edited

Loading