reddit / baseplate.go

Reddit's Service Framework in Go

This repository is archived.

This repository is archived and will not receive any updates or accept issues or pull requests.

To report bugs in reddit.com please make a post in /r/bugs.

If you have found a bug that can in some way compromise the security of the site or its users, please exercise responsible disclosure and e-mail [email protected].

---

API

For notices about reddit API changes and discussion of reddit API client development, subscribe to the /r/redditdev and /r/changelog subreddits.

To learn more about reddit's API, check out our automated API documentation and the API wiki page. Please use a unique User-Agent string and take care to abide by our API rules.

Quickstart

To set up your own instance of reddit see the install guide.

baseplate.go's Issues

`retrybp.NetworkErrorFilter` would also return true for `context.DeadlineExceeded`

See: https://play.golang.org/p/wGfQvUejHQ7

Upgrade go-redis to a non-beta version

Provide an effective configuration file

Hello, I'm trying to run the thrift server example in this repo. thriftbp/example_server_test.go

But got some errors related to configuration files, please help me.

1. No config file panic

I run the server by go run server.go, i get a panic error

panic: no $BASEPLATE_CONFIG_PATH specified, cannot load config

goroutine 1 [running]:

which indicating that no $BASEPLATE_CONFIG_PATH environment variable is specified, which means the server cannot load the necessary configuration file. To fix this error, I specify the path to the configuration file by setting the BASEPLATE_CONFIG_PATH environment variable to the path of the configuration file, like this:

BASEPLATE_CONFIG_PATH='example/server_cfg.yaml' go run server.go

example/server_cfg.yaml is copy from unittest baseplate_test.go, and I delete the redis config.

2. secrets.json not exist panic

Then, I run the server with BASEPLATE_CONFIG_PATH='example/server_cfg.yaml' go run server.go

The server starts normally, But a few seconds laster, the server panic, and I got following error

ø> BASEPLATE_CONFIG_PATH='example/server_cfg.yaml' go run server.go
2023-03-24T17:23:06.864+0800    DEBUG   Parsed configuration as *baseplate.Config:
addr: :8080
timeout: 30s
stopTimeout: 30s
log:
 level: info
runtime:
 numProcesses:
  max: 100
secrets:
 path: /tmp/secrets.json
tracing:
 namespace: baseplate-test
 queueName: test
 recordTimeout: 1ms
 sampleRate: 0.01
        {"pre_init": true}
maxprocs: Leaving GOMAXPROCS=12: CPU quota undefined
panic: baseplate.New: failed to init secrets: filewatcher: context canceled while waiting for file(s) under "/tmp/secrets.json" to load: errorsbp.Batch: total 2 error(s) in this batch: context deadline exceeded; last error: filewatcher: i/o error: stat /tmp/secrets.json: no such file or directory (config: secrets.Config{Path:"/tmp/secrets.json"})

which means I need to provide a valid secrets.json config file. What's the format of secrets.json, can you give me an exmaple, and explain it's meaning.

Add a tutorial to the documentation

We should add a "tutorial" to baseplate.go like what we have for baseplate.py:

https://baseplate.readthedocs.io/en/stable/tutorial/index.html

Easier to use mock secrets store

Currently whenever we need to use secrets store in test code, we usually need to put a whole json string, and create a temp directory and do a lot of heavy lifting to get it usable. An easier to use mock version would make lives easier. Ideally it should avoid the need of creating a temp directory, but just pack whatever we are doing right now into a function to make it reusable would already help a lot.

Secret callback

When fetched secret is used for creating a downstream client, it's possible that we need to create a new client once the secret is updated. Add capability of being notified of updated secret via secret store will allow us to renew a client as needed.

Maybe something like

func NewStore(ctx context.Context, path string, logger log.Wrapper, notify chan interface) (*Store, error) {
	parser := func(r io.Reader) (interface{}, error) {
		secrets, err := NewSecrets(r)
		if err != nil {
			return nil, err
		}
                notify <- secrets
		return secrets, nil
	}
        ...

Also open for other suggestions.

Baseplate Message Signature

Baseplate spec link. It should be trivial to implement once we had secrets store implementation.

Remove deprecated stuff

We need to remove the following already deprecated stuff before the release of 0.9.0:

1. redisbp package
2. Metrics from span hooks from the old baseplate spec
3. batchcloser.CloseError
4. metricsbp.StatsdConfig.CounterSampleRate
5. retrybp.PoolExhaustedFilter
6. retrybp.UnrecoverableErrorFilter

Add an io.Closer implementation that closes a collection of closers

There are a couple of places where we are already gathering a bunch of closers together under a single io.Closer that closes all of them when Close() is called, we should just formalize this into an exported struct.

Reason for suffix 'bp' behind packages name.

Can any one elaborate reason behind bp suffix when this packages can be imported without it. And if some of those package have same name with the repo(in which baseplate is imported) can be imported with alias.

Write to posix mq

This is needed by (and blocking) both #2 and #4.

A few options are:

1. Use third party library directly (examples: 1, 2), which all requires cgo
2. Write a pure go library to handle the writing part with syscall
3. Have a separated sidecar that pumps from unix socket to mq (using third party libraries from option 1), and isolate that in its own process so that microservices do not need to link against the cgo library (they only need to write to the unix socket by the sidecar)

Edge request context

spec link, baseplate.py implementation link

Change `log.Wrapper.UnmarshalText` to use `ErrorWithSentryWrapper` as the default one

In order to support #364, we also need to change log.Wrapper.UnmarshalText to use ErrorWithSentryWrapper instead of NopWrapper as the default one.

Add Prometheus metrics endpoint

In order to provide support for services being monitored by Prometheus we need to add the ability to configure a Prometheus /metrics endpoint.

The https://github.com/prometheus/client_golang library is recommended.

This would only be useful for registered metrics "tagged" mode.

Add standard test EdgeRequestContexts

One problem we almost always run into when writing tests for baseplate services is having to create sever edge request context headers to test our endpoints. It would be nice if baseplate could provide a standard collection of these values that could be used by each service.

thrifttest.NewBaseplateServer should accept an ecinterface.Interface

Right now thrifttest.NewBaseplateServer always uses a mock ecinterface.Interface:

baseplate.go/thriftbp/thrifttest/server.go

Line 188 in b6e625c

EdgeContextImpl: ecinterface.Mock(),

This doesn't work if you are trying to pass around an edge context in tests, it would be good if you could pass in an actual implementation that can decode a real edge context.

Enable strict yaml parsing

In baseplate.New:

1. If the passed in extra config is nil, turn on strict mode on the parsing of baseplate.Config
2. If the passed in extra config is non-nil, allow user to pass in another boolean arg to indicate whether they want to enable strict parsing for their extra config. If they pass in true for it, they must add baseplate.Config into their config struct with yaml:",inline" tag so it could pass strict mode.

A piece of dead code

The following code never gets executed, since the err != nil case is handled above.

baseplate.go/thriftbp/client_pool.go

Lines 362 to 367 in b288626

	if err != nil && errors.As(err, new(net.Error)) {
	// Close the client to avoid reusing it if it's a network error.
	if e := client.Close(); e != nil {
	log.Errorw("Failed to close client", "origErr", err, "closeErr", e)
	}
	}

cc @fishy

Events

The baseplate.go implementation would only need to send serialized event bytes to posix message queue. We could reuse baseplay.py implementation to read from posix message queue and actually send them to our backend.

For the serialization part, baseplay.py’s reference implementation is available. We would only need to implement the v2 part as v1 events are going to be deprecated very soon. For the sending to posix message queue part, there are third-party go libraries available: 1, 2.

Add more sophisticated Logger

Implement logging other that standard log package

What is Baseplate?

The README states that this is

Baseplate implemented in Go

What is Baseplate? It would be helpful if the original Baseplate was linked. It sounds like this is an alternative implementation of some other thing called "Baseplate". If that other thing is not publicly available it would help to add a generic description to the README which explains what people are looking at.

Tracing

This mainly depends on THeader support (DONE). Details TBD but baseplate.py’s reference implementation is available.

Top level doc comment

Add a top level doc.go and doc_test.go (at the root directory) to give users a high level overview of how to use the packages, how to manage their main function, etc.

Experiments

Details TBD. baseplate.py reference implementation, baseplate spec

Users bucketing into experiments not working

Users are not getting distributed properly in buckets:

• For the single_variant experiment type, 100% of users always fall into the second bucket.
• For the multi_variant, feature_rollout, and range_variant experiment types, 100% of users always fall into the "no treatment" bucket.

Change edgecontext package to support non-singleton usage

Remove the `Config` types that's solely for yaml parsing

We currently have a few Config types defined in packages that their sole purpose if for yaml parsing, for example tracing.Config. We should remove them and just make the actual config type used by the code (for example, tracing.TracerConfig in this case) for yaml parsing.

Prometheus `${protocol}_success` label has different behavior depending on the protocol

Right now ${protocol}_success is used as a stringified boolean true | false. However for user errors (4xx), the http instrumentation logs them as a success, but the thrift instrumentation logs them as a failure.

At the very minimum we should ensure that both have the same behavior.

Since 4xx never really fits completely in errors nor success, I suggest that we remove the ${protocol}_success label and instead replace it with a new label ${protocol}_status which would have one of the following values success | user error | app error

Opinion?

Update redisbp to use v8 of go-redis

We actually started this, just never finished it: #258

`thriftbp.ClientPool` should not implements `thrift.TClient`

As discussed with #359, making thriftbp.ClientPool implementing thrift.TClient leads to footgun that services creating a concrete service client out of it and share that across multiple goroutines, which would lead to data races.

Adding a function like thriftbp.ClientPool.TClient (name tbd) to return an thrift.TClient implementation instead, so forcing services sharing the created service client to fix their code.

Cookiecutter

This would be a command line program to generate a baseplate.go project skeleton code, with necessary go-kit parts filled in. Details TBD.

Upgrade go-kit to v0.10.0

Looks like a new version of go-kit was released, we should upgrade our pinned version:

https://github.com/go-kit/kit/releases/tag/v0.10.0

Support HTTP services

#67

• Add httpbp package to provide the base helpers to get headers from the HTTP request and put them on the context object.

• Add function to build a new EdgeContext from HTTP headers

• Add go-kit Middleware to inject an EdgeContext into the context

• Add function to build a new Span from HTTP headers

• Add go-kit Middleware to inject a server Span into the context

Add a method for creating test thrift servers, similar to httptest.NewServer

Move experiments package to its own repo

Similar to Baseplate.py 1 2 and edgecontext, we should move experiments package out of Baseplate.go so it can be updated on its own cadence.

Since this will be a breaking change, 0.9.0 would be a good opportunity to do it.

edgecontext: Switch to use builtin key rotation in jwt package

Depends on dgrijalva/jwt-go#372