nytimes / gizmo Goto Github PK

When prefix is empty mux failed to add subroutes. It does not return any kind of errors, just silently fail.

I already made a fix:
https://github.com/gaplyk/gizmo/blob/master/server/simple_server.go#L180
Can create pull request if this is will be considered as an issue.

Since a prefix can an optional value for services, we should not force everyone to implement it, especially since the value is likely to be immutable.

We should remove the method from the server.Service interface, include it's value as an attribute in config.Server and then change the server.Register implementations to utilize it.

THIS WILL BE A BREAKING CHANGE!
If possible, it'd be nice if we could provide a small utility that would see a Prefix() implementation, remove it and then add a line about server.Init(...) in main.go to do a config.Server.Prefix = "xyz" on the line above.

We have far too much documentation in the README, which makes keeping things up to date rather difficult.

We should trim down the README to just cover the highlights and purpose of gizmo.

Still playing with the RPC example I have discovered that the server.MonitorRPCRequest() does not write to the RPC log file.

Any hint about a possible solution ? Thx

Go Kit's metrics package is stable, more active and has support for statsd and prometheus.

Currently in NewSubscriber(cfg SQSConfig) (pubsub.Subscriber, error) there are only 2 ways the Credentials are set: Static and from the Environment.

I would need to use the AssumeRoleProvider in order to be able to switch to the role authorised to use the service

The toDelete channel in the SQSSubscriber should not close the channel on Stop() as users may need to finish processing messages before shutting down.

Currently, I am implementing kind of message bus for each service.
I would like to see some example on how we should integrate pubsub into Service such as JSONService
If anyone can provide some sort of README or example. I would very much appreciate it :D

The App Engine Set Up (such as tracing, metrics, and logging) does not have to be locked with server/kit, this way you can use it with the SimpleServer

The Go Cloud Development Kit, an open-source product of Google's Go team, is a set of high-level APIs that are designed to work across multiple cloud and on-prem providers. For example, our blob package lets the same code read and write data to Amazon S3, Google Cloud Storage, Azure Storage or a local filesystem.

We'd like to integrate our Go CDK pubsub API with Gizmo. We'd add a pubsub/gocdk directory with implementations of the four pubsub interfaces and config support. Gizmo would immediately gain two pubsub implementations—Azure and RabbitMQ—and would benefit from any future Go CDK integrations.

In #156 functionality was added to be able to override the AWS endpoint so that a mock AWS implementation could be used.

The SNS constructor was updated but it looks like the SQS constructor was not.

It looks like a pretty simple change so will open a PR with a proposed fix soon

I don't write a lot of go. I feel like there's something critical I'm missing in being unable to run this program, it doesn't even include a service. Any help would be appreciated.

I installed go from brew

❯ go version
go version go1.9.1 darwin/amd64

I'm attempting to use dep to install dependencies. So I ran dep init and dep ensure with the example below.

package main

import (
	"github.com/NYTimes/gizmo/server"
)

func main() {
	server.Init("test-server", nil)
	if err = server.Run(); err != nil {
		server.Log.Fatal("unable to run server: ", err)
	}
}

❯ go build main.go
# test/vendor/github.com/NYTimes/gizmo/server
vendor/github.com/NYTimes/gizmo/server/router.go:87:17: cannot use h (type http.Handler) as type http.HandlerFunc in assignment: need type assertion

Is there anything I can do on my end to fix this or is this really an issue in gizmo?

We should have notes in the examples/pubsub README to detail how to:

1. Creating an SNS, SQS, subscribing one to the other and enabling 'raw delivery'.
2. Setting up a local Kafka instance.

Since we've added the Router interface and the httprouter, Gizmo has added a new, shared convenience function for accessing request parameters:

https://godoc.org/github.com/NYTimes/gizmo/web#Vars

We should change all of the examples (and the wiki!) to using this func.

Hi,

I'm just wondering what your code of conduct is for this project?

Thanks,
Alison

The localstack project provides local emulation of AWS services that is very useful for testing. The AWS SDK provides Config.Endpoint which can be set to the localstack URL for this purpose.
Since gizme completely masks the config structure, the Endpoint field should be added to the gizmo aws.Config struct.

Maps do not allow users to declare the order in which their endpoints are registered against our routers, which can force developers to compromise how their URIs are constructed or even force users to build custom router implementations.

By switching to an array, users will be allowed to declare the order in which routes will be registered. Long term goal here is to eventually open up a path for us to remove any custom routing options in the future and purely rely on gorilla/mux.

I haven't figured out exactly what this API will look like, but I hope the change to be simple enough that minimal refactoring is required for the upgrade.

It would be nice if you can add tag releases. We are using dep but I don't want to vendor the master branch. Thanks.

Some breaking changes were recently introduced in the Google PubSub client: https://groups.google.com/forum/#!topic/google-api-go-announce/8pt6oetAdKc/discussion

We need to update our client to match the changes, which utilize a new Receive function instead of the existing iterator: https://godoc.org/cloud.google.com/go/pubsub#example-Subscription-Receive

https://github.com/NYTimes/gizmo/blob/master/server/server.go#L121
Right now we are not able to use JSON formatter if we do not set logging to file.

In order to be able to use the attribute based filtering, would be nice if the Publisher allowed to specify the MessageAttributes field in sns.PublishInput so basically have something like:

// PublishRaw will emit the byte array to the SNS topic.
// The key will be used as the SNS message subject.
func (p *publisher) PublishRaw(_ context.Context, key string, m []byte, messageAttributes map[string]*MessageAttributeValue) error {
	msg := &sns.PublishInput{
		TopicArn: &p.topic,
		Subject:  &key,
		Message:  aws.String(base64.StdEncoding.EncodeToString(m)),
		MessageAttributes: messageAttributes,
	}

	_, err := p.sns.Publish(msg)
	return err
}

ref. https://aws.amazon.com/blogs/compute/simplify-pubsub-messaging-with-amazon-sns-message-filtering/

I am developing an API with gizmo microservice toolkit. One of use cases is a sudden load spike for short period of time like 4 hours 2 days per week. For the use case, I am trying to optimize performance of the API in various parts as much as I can. A a part of the optimization, I would like to incorporate HttpRouter in gizmo Sever. Based on the benchmarks of various Go http routers (https://github.com/julienschmidt/go-http-routing-benchmark), HttpRouter shows very good performance with very low heap allocation rate. At this point, SimpleServer uses Gorilla mux as a http router. To use HttpRouter, it needs FastSimpleServer , which is planned this year per http://open.blogs.nytimes.com/2015/12/17/introducing-gizmo. I am looking forward to adding FastSimpleServer with HttpRouter to gizmo for those who want HttpRouter as a http router.

panic: descriptor Desc{fqName: "rpc.Data.DURATION", help: "rpc.Data.DURATION", constLabels: {}, variableLabels: []} is invalid: "rpc.Data.DURATION" is not a valid metric name

goroutine 1 [running]:
github.com/prometheus/client_golang/prometheus.(*Registry).MustRegister(0xc420078980, 0xc4201e35a0, 0x1, 0x1)
	/Users/207269/go/src/github.com/prometheus/client_golang/prometheus/registry.go:353 +0x92
github.com/prometheus/client_golang/prometheus.MustRegister(0xc4201e35a0, 0x1, 0x1)
	/Users/207269/go/src/github.com/prometheus/client_golang/prometheus/registry.go:152 +0x53
github.com/go-kit/kit/metrics/prometheus.NewSummaryFrom(0x0, 0x0, 0x0, 0x0, 0xc4201e6fc0, 0x11, 0xc4201e6fc0, 0x11, 0x0, 0x0, ...)
	/Users/207269/go/src/github.com/go-kit/kit/metrics/prometheus/prometheus.go:99 +0xe8
github.com/go-kit/kit/metrics/provider.(*prometheusProvider).NewHistogram(0xc4201e6580, 0xc4201e6fc0, 0x11, 0x32, 0x9, 0xc4201e6fc0)
	/Users/207269/go/src/github.com/go-kit/kit/metrics/provider/prometheus.go:59 +0x107
github.com/NYTimes/gizmo/server.registerRPCMetrics(0xc4201e3548, 0x8, 0x17eb2c0, 0xc4201e6580)
	/Users/207269/go/src/github.com/NYTimes/gizmo/server/rpc_server.go:298 +0x102
github.com/NYTimes/gizmo/server.(*RPCServer).Register(0xc42006e9b0, 0x17e7340, 0xc4201e3550, 0xc4201e3088, 0xc4201e3090)
	/Users/207269/go/src/github.com/NYTimes/gizmo/server/rpc_server.go:78 +0x123
github.com/NYTimes/gizmo/server.Register(0x17e7340, 0xc4201e3550, 0xc420061380, 0xc420082098)
	/Users/207269/go/src/github.com/NYTimes/gizmo/server/server.go:130 +0x49
main.main()

Needs similar fixing like we did here: #69

Is there a reason why gizmo calls Middleware on every single request here? https://github.com/NYTimes/gizmo/blob/master/server/simple_server.go#L114

Why shouldn't the Middleware method be called once during server.Register so that external handlers only have to be instantiated once such as https://godoc.org/go.opencensus.io/plugin/ochttp#example-Handler--Mux

The next release should drop support for anything below 1.9 now that 1.12 is about to be released. This way we can clean up SimpleServer a little bit to get rid of the build tags.

Usage example of the GCP is missing in the examples folder.

To allow users to inject a custom health check handler, we should add a CustomHealthCheckHandler http.Handler attribute to config.Server.

If the config.Server.HealthCheckType is "custom", the NewHealthCheckHandler should look for the CustomHealthCheckHandler function and use it.

While trying to run the sqs-sub pubsub example, I keep encountering this error:

InvalidAddress: The address https://sqs.us-east-1.amazonaws.com/ is not valid for this endpoint.
status code: 404, request id: 14c12121-5d1c-5583-a35d-73d68936cbf6dmcdonald@dmcdonald-mbp15:~/go/src/dmcdonald/pubsubspike/

This traces to the s.sqs.GetQueueUrl call. I tried calling this api method myself with the following code, and got success:

	cfg := aws.NewConfig().WithCredentials(
		awscredentials.NewEnvCredentials(),
	).WithRegion(region)
	sqsClient := sqs.New(session.New(cfg))
	if resp, err := sqsClient.GetQueueUrl(&sqs.GetQueueUrlInput{QueueName: &queueName}); err != nil {
		fmt.Printf("failed to get queue url: \n%v\n", err)
	} else {
		fmt.Println(resp)
	}

I'm using the same permissions, region and queuename with both the gizmo example and my own test code.

The error response indicates possibly a permissions issue, but I can't seem to reproduce it outside of the gizmo codebase. Any suggestions on how to troubleshoot this further?

Would it be possible to up the tag of Gizmo to v0.0.2? Would love to “properly” update our dependencies to fix the panic on 404s. Thanks

We need to add AddPermissionWithContext: https://travis-ci.org/NYTimes/gizmo/jobs/217293105

Right now when panic is printed to stderr, there is no way to add any additional information, like headers or cookies.

metadata.FromContext has been removed from grpc/grpc-go#1392

It's used in gizmo/server.

According to net/http documentation, the server never needs to close a body request, see
https://golang.org/src/net/http/request.go?s=5615:5823#L96

So this whole defer statement is unnecessary: https://github.com/NYTimes/gizmo/blob/master/server/simple_server.go#L107:L113

Hi,

One more thing i came across: the gcp.Publish and gcp.PublishRaw have a param key string.

However this is converted to Attributes: map[string]string{"key": key} here. Wouldn't it be better to allow a map in the params of those functions?

Would of course mean a breaking change, but the functionality is partially broken now anyways?

Hi,

When trying to implement a CORS handler that catches all OPTIONS requests I noticed that the current Middleware wraps all requests that are defined in the Endpoints function. Would it be possible to allow wrapping of the mux.ServeHTTP func somehow?

When using httprouter instead of Gorilla at least the OPTIONS requests are handled by the router itself but there is no way of adding allowed origins to that response.

Prehaps a RouterMiddleware function that allows you to wrap the mux's ServeHTTP.

What are your thoughts on this?

Rather than rely on a MasterHost config value, which can change during elections, it would be more flexible to use Mongo's built in support for read preference and tags to constrain which nodes in the Hosts field can be used.

If these configurations are set, the MongoDB object can supply these to the mgo Session object before returning it to the client via:

https://godoc.org/gopkg.in/mgo.v2#Session.SetMode (Read Preference)
https://godoc.org/gopkg.in/mgo.v2#Session.SelectServers (Tags)

I'm not sure how many folks at the Times are using gizmo but if you think it would be useful maybe we could add a request id header (X-NYT-Request-Id) to gizmo? If that's too Times specific maybe we could use X-Request-ID like Heroku does (https://devcenter.heroku.com/articles/http-request-id)? This header would be parsed from requests coming into a service and passed by the service to other services in HTTP requests that it makes. It would also be logged with all log statements related to a particular web transaction.

Somehow TimedAndCounted here is passed the full request path INCLUDING the GET parameters which messes up the labels pretty good.

Not sure when that started or if that has always been the case but we should strip GET parameters from the string before using it as a label.

As a result of #129 the Prometheus metrics for an endpoint are now recorded with the actual URL accessed, not the URL path that was registered.
This is a problem when the registered URL path contains regular expressions like id:[0-9]+ cause the label will contain the actual ID and the stats won't get collapsed into the same time series.

Actually, if I'm not mistaken then #129 broke all metric recording, not only Prometheus style metrics, because the fullPath is also used in the default section here: https://github.com/NYTimes/gizmo/blob/73db46d2c901dfd0cb3f45d67bc75c1a248eb685/server/metrics.go#L145-L147

gorilla/mux has build tags that opt to use the new/native context package for 1.7.

Unfortunately, Gizmo's server.Router implementation uses gorilla/context under the hood to allow httprouter to pass parameters. This leads to a massive memory leak when running with 1.7 as the gorilla/context never gets cleared.

Hi,

Since logrus changed it's case (github.com/Sirupsen/logrus to github.com/sirupsen/logrus) using Gizmo can cause a case-insensitive import collision. This happens when both github.com/Sirupsen/logrus and github.com/sirupsen/logrus are imported.

As stated on the logrus github page:

Seeing weird case-sensitive problems? Unfortunately, the author failed to realize the consequences of renaming to lower-case. Due to the Go package environment, this caused issues. Regretfully, there's no turning back now. Everything using logrus will need to use the lower-case: github.com/sirupsen/logrus. Any package that isn't, should be changed.

I'll submit a pull request with the simple fix of renaming Sirupsen to sirupsen.

The Google Cloud API client libraries for Go are making some breaking changes:

• The import paths are changing from google.golang.org/cloud/... to
  cloud.google.com/go/.... For example, if your code imports the BigQuery client
  it currently reads
  import "google.golang.org/cloud/bigquery"
  It should be changed to
  import "cloud.google.com/go/bigquery"
• Client options are also moving, from google.golang.org/cloud to
  google.golang.org/api/option. Two have also been renamed:
  • WithBaseGRPC is now WithGRPCConn
  • WithBaseHTTP is now WithHTTPClient
• The cloud.WithContext and cloud.NewContext methods are gone, as are the
  deprecated pubsub and container functions that required them. Use the Client
  methods of these packages instead.

You should make these changes before September 12, 2016, when the packages at
google.golang.org/cloud will go away.

Hi,

I'm trying to use the GCP Pubsub throught github.com/NYTimes/gizmo/pubsub/gcp but my receiver is running an import with a long duration surpassing the defaultMaxExtension = 60 * time.Second as defined on gcp.go:51. Causing the message to be resend because the receiver failed to Ack() the message.

Maybe i'm missing something but i don't see a way to change the MaxExtension on the underlying subscription's ReceiveSettings

I guess this requires a change in the code to make customisable? I'm happy to send in a pull request but would like to hear if it's needed an how you would like to see it implemented?

We should create an flag in config.Server to tell Server implementations to expose metrics via JSON endpoints via expvar using the subpackage from go-metrics: https://godoc.org/github.com/rcrowley/go-metrics/exp

This will enable users to utilize metrics stacks based around 'pulling' data instead of just the 'push' style of Graphite.

Hello,

I have looked examples folder but all examples has single service. So how can I organize folder multiple services.

Can I define multiple services in main.go file? if I can, what about service's config.json file ?

Sorry my bad english.

I cloned this repo and ran make build. It failed with

verifying github.com/influxdata/[email protected]: checksum mismatch
        downloaded: h1:L2DsZmSjI2ySLb1AaYtUkBjMcgRZC1DsL9fcagszDv4=
        go.sum:     h1:ekuRYg65KyXed1eWHMYKBNfa8Zw57UbBef4RTKPxiLY=

I just cloned the RPC example but the HTTP endpoints are not exposed.

I also had to modify the code because in newer versions of gRPC you have to add grpc.WithInsecure() when calling grpc.Dial(...)

Any clue why the http endpoints are not being exposed ?

#18 brought up the issue that there is nothing to prevent users from registering multiple services on a single Gizmo server, which I think is against the microservice philosophy.

To force users to only have 1 Service per Server, we should change the 'Register' methods to error if a service has already been registered.

grpc supports various configurable options when creating a new rpc server.

func NewServer(opt ...ServerOption) *Server

It would be nice if these options can be managed via a new Config type or some other solution.

I can work on this if it sounds reasonable.