[ADDED] Service api improvements #1160

piotrpio · 2022-12-12T12:48:06Z

Changes to service API:

comply with ADR
change package name (services -> service)
add custom callback handlers: DoneHandler and ErrorHandler
add wrapper around nats.Msg (Request) to enable adding helper methods
change Service to a struct (instead of interface)
add documentation and examples
add more tests
general cleanup

Git diff unfortunately displays this diff as added/removed files, because of the changed package/dir name (I wanted to make it singular).

coveralls · 2022-12-12T12:53:28Z

Coverage increased (+0.006%) to 85.528% when pulling 63932a2 on service-api-improvements into 95a7e50 on main.

derekcollison · 2022-12-12T18:18:18Z

service/example_package_test.go

+	// Multiple instances of the servcice with the same name can be created.
+	// Requests to a service with the same name will be load-balanced.
+	for i := 0; i < 5; i++ {
+		svc, err := Add(nc, config)


This is just Add now? I think we might be simplifying too much here.

It's in separate package now, so the way you actually use it is by calling service.Add(), which I think is really intuitive and pretty much a standard in Go (e.g. list.New() or context.WithTimeout(), http.Server() etc.).

In that sense, service.AddService() seems redundant.

I thought you would hang it off alive connection. nc.AddService. This is how we register interest with a connection, and that is what a service it imo. Do you now make them pass in a nats.Connection?

Yes - I want to separate services to a separate, importable package - if user does not want to use service framework, the API is less cluttered and they do not see methods they do not need. After importing service package and typing service. in IDE, you'll see all you need to start using interfaces. It's self-documenting and easier to use.

We should discuss, we had stated we did not want this as a separate package IIRC because we wanted this to be the default way to do micro-services etc.

Let's schedule some time to discuss before this gets merged.

The part where nc.AddService may come in handy is when you have multiple services on top of the same connection and you want to be able to use the error handling on a per service basis, since nc.Opts.AsyncErrorCB belongs to the connection, in current PR it seems that only one Service would be able to use the error handler.

Current service implementation also has ErrorHandler, which extends AsyncErrorCB.

Not a fan of passing in nc tbh. I believe I prefer nc.AddService() tbh.

This is the kind of package that will sprawl with dependencies like observability and more over time. Might be worth keeping in a seperate package to avoid those being in nats.go core

@ripienaar You have a good point here, but unfortunately, separate packages have the same go.mod.

We would need a multi-module setup. That brings it's own problems and complexity (especially with incompatible versions of dependencies).

derekcollison · 2022-12-12T18:20:30Z

service/service.go


-	// Service is an interface for service management.
-	// It exposes methods to stop/reset a service, as well as get information on a service.
-	Service interface {


Why change away from an interface?

I was wondering whether to better have interface or not, but my general reasoning is:

we don't really need an interface here - unlike in JetStream simplification, where we'll probably want to support Push and Pull consumers separately (and possibly ordered consumer as well) on one interface, here we have 1 implementation

by not using interfaces, we enable other users to add their own interfaces on top of Service() however they want, and we do not break anything for them if we want to add more methods

core NATS client does not rely on any interface, so while it's not 100% consistent (JesStreamContext is an interface), we do not break conventions

I think lately folks have pushed us more towards interfaces. I agree we should optimize for extensibility.

@ripienaar What are your thoughts?

If you want people to wrap it or augment with additional abilities without changing code then interfaces help. So for KV for example its an interface and we can wrap a cache, codec, e2e encryption or whatever we like - those just wrap the interface and any other calling code doesnt need to change.

So if that kind of thing isnt a goal here then you can be more fluid about it.

Regarding core nats package - Msg is a great example of someting that really should have been an interface. Rather than the way things are now with a Msg struct with a ton of methods on it thats just never used by core nats

interface = can change internal details or have alternative implementations = win.

derekcollison · 2022-12-12T21:59:02Z

I suggest we have it as a separate package, but similar to JetStream its constructor is in the core pkg.

So nc.AddService().

I also think its best that service goes back to an interface.

/cc @ripienaar @aricart

service/service.go

wallyqs · 2022-12-12T22:12:58Z

service/service.go

+	svc.natsHandlers.asyncErr = nc.Opts.AsyncErrorCB
+	if nc.Opts.AsyncErrorCB != nil {
+		nc.SetErrorHandler(func(c *nats.Conn, s *nats.Subscription, err error) {
+			if config.ErrorHandler != nil {


I think here you need to check whether the s *nats.Subscription belongs to one of the subscriptions that were mounted by the Service (reqSub, etc...), as the error handler would apply to any other subscription from the client.

Good catch - done.

wallyqs · 2022-12-12T22:16:47Z

service/service.go

+					Subject:     s.Subject,
+				})
+			}
+			svc.Stop()


should this one be run in a goroutine? What could be interesting here is to check whether the error is a SlowConsumer error for example to have it drain, and maybe let the service resubscribe once its buffers have caught up.

I'm not sure whether it should run in a goroutine... It is added in async handler anyway, I would probably keep it synchronous within one callback.

As to detecting e.g. slow consumer error (maybe others), I would do that in a separate PR if that's ok, just to make this one works properly and then add improvements.

piotrpio · 2022-12-13T15:07:26Z

@derekcollison I agree that changing Service back to an interface may be useful, so I'll do that.

As to having constructor as a method on *nats.Conn (nc.AddService), that's harder because we need to deal with cyclic dependencies (which go does not allow). The whole structure of what we want to achieve suggests that core nats connection can be created without any knowledge of a service, but service cannot be created without knowledge of core nats connection. To me, that classifies it as a separate package, dependent on core nats.

Creating a wrapper around an existing type, even reusing some kind of connection in it, by passing the connection as an argument is not uncommon in go, neither in standard library nor in other popular libraries. Some examples:

crypto/tls takes net.Conn as argument to create new TLS connection:

func Client(conn net.Conn, config *Config) *Conn

net/smtp package uses net.Conn to create a new smtp client:

func NewClient(conn net.Conn, host string) (*Client, error)

gorilla/websocket (https://github.com/gorilla/websocket), the most popular go websocket library, also uses net.Conn to create a websocket connection:

func NewClient(netConn net.Conn, u *url.URL, requestHeader http.Header, readBufSize, writeBufSize int) (c *Conn, response *http.Response, err error)

Ultimately, we do a similar thing - we create a new layer on top on nats.Conn in order to enhance capabilities.

wallyqs · 2022-12-13T16:22:58Z

@piotrpio that's a good point on cyclic dependencies, it will not be possible to separate concerns into two packages and also have something like nc.AddService(&service.Config{}) since the queue subscription async handler has to be a concrete type like func(msg *Msg). If we had way to be able to create async callbacks in the core client without using concrete types (e.g. an interface Msg type), then you would be able to abstract most of the client and subscription behavior into smaller interfaces as needed and then be able to separate into two packages.

piotrpio · 2022-12-13T16:31:54Z

@wallyqs yeah, I was thinking about relying on interfaces in service package, but because of the concrete types (message handlers, but also *nats.Subscription being returned by Subscribe()), that will not be possible...

derekcollison · 2022-12-13T18:12:46Z

ok we can discuss, but feel it should be nc.AddService

wallyqs · 2022-12-14T06:15:30Z

to get to nc.AddService and still be able to have another package, we could add a few extra small interfaces to the service package that represent the needed behavior from the client, then implement the internals of service package based on those: b169849

piotrpio · 2022-12-14T11:12:37Z

@wallyqs Yeah, I tried that approach yesterday and while it's doable, I don't like it for a couple of reasons:

Even though the interface implementation would be hidden in core nats client, we would still need to expose those extra interfaces in services package. For experimental phase that would be OK I assume, but we don't know how long this will be beta and when nats client will support interfaces by default (which would allow us to remove them eventually). I don't like exporting interfaces which make sense only for one, internal use case.
I'm worried about the development and maintanance of services if we basically split the logic across 2 packages. In the future API will be getting new features, and a lot of them will require adding and implementing new methods for this interface in core nats package. I'm worried that this will become very cumbersome to maintain + hard to read and understand if some of the newer folks will want to contribute.
The interfaces you proposed in your POC are minimal, but in reality we would have to add at least Subscribe(), but also some methods to be able to modify (extend and restore) callbacks for core client (cancel handler and async error handler). The logic for those handlers would probably have to reside in nats package.

wallyqs · 2022-12-14T21:29:55Z

micro/service.go

+		response, _ := json.Marshal(svc.Info())
+		if err := req.Respond(response); err != nil {
+			if err := req.Error("500", fmt.Sprintf("Error handling INFO request: %s", err)); err != nil && config.ErrorHandler != nil {
+				go config.ErrorHandler(svc, &NATSError{req.Subject, err.Error()})


This would spin up as many go routines as errors appear, in the nats.go these are serialized over a channel to be able to have some control over these: https://github.com/nats-io/nats.go/blob/main/nats.go#L2769-L2792

Changed implementation to use an error dispatcher per-service

wallyqs · 2022-12-14T21:32:13Z

micro/service.go

+		response, _ := json.Marshal(ping)
+		if err := req.Respond(response); err != nil {
+			if err := req.Error("500", fmt.Sprintf("Error handling PING request: %s", err)); err != nil && config.ErrorHandler != nil {
+				go config.ErrorHandler(svc, &NATSError{req.Subject, err.Error()})


maybe could use err error as part of the state from NATSError? That way later on could be able to use errors.Is(err, ErrSlowConsumer) and handle that case for example

I would leave it as it is for now, ErrSlowConsumer would be handled internally anyway. We could add it later on if needed.

micro/service.go

Co-authored-by: Tomasz Pietrek <melgaer@gmail.com>

Jarema

LGTM!

piotrpio requested review from Jarema, aricart and wallyqs December 12, 2022 12:50

derekcollison reviewed Dec 12, 2022

View reviewed changes

wallyqs reviewed Dec 12, 2022

View reviewed changes

service/service.go Outdated Show resolved Hide resolved

wallyqs reviewed Dec 12, 2022

View reviewed changes

piotrpio added 7 commits December 14, 2022 18:54

Service API improvements

e0446b0

Add examples and documentation

11dd386

Change Service implementation to a struct

11f2998

Fix data races

2ae2578

Revert to using service interface, rename package to micro

7d5cf4a

Adopt ADR changes

972dcd2

Use getters to retrieve async handlers

d3ae7e5

piotrpio force-pushed the service-api-improvements branch from dc44f93 to d3ae7e5 Compare December 14, 2022 17:57

wallyqs reviewed Dec 14, 2022

View reviewed changes

piotrpio added 2 commits December 15, 2022 12:19

Add callbacks dispatcher

5353379

Add licence

7ae2834

piotrpio force-pushed the service-api-improvements branch from de7a8a3 to 7ae2834 Compare December 15, 2022 15:34

Add setting error payload

65046c9

piotrpio added 2 commits December 15, 2022 18:04

Return error from handler to be used in stats

1f3fcdc

Fix schema handler

ade30c4

piotrpio requested a review from wallyqs December 19, 2022 09:01

Jarema reviewed Dec 20, 2022

View reviewed changes

micro/service.go Outdated Show resolved Hide resolved

Fix comment on AddService

d1e8c98

Co-authored-by: Tomasz Pietrek <melgaer@gmail.com>

piotrpio requested a review from Jarema December 20, 2022 08:27

Jarema approved these changes Dec 20, 2022

View reviewed changes

Switch to using json.RawMessage for stats data

63932a2

piotrpio merged commit 32b9daa into main Dec 20, 2022

piotrpio deleted the service-api-improvements branch December 20, 2022 11:52

piotrpio mentioned this pull request Dec 20, 2022

Release v1.22.0 #1165

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ADDED] Service api improvements #1160

[ADDED] Service api improvements #1160

piotrpio commented Dec 12, 2022 •

edited

coveralls commented Dec 12, 2022 •

edited

derekcollison Dec 12, 2022

piotrpio Dec 12, 2022

derekcollison Dec 12, 2022

piotrpio Dec 12, 2022

derekcollison Dec 12, 2022

wallyqs Dec 12, 2022 •

edited

piotrpio Dec 12, 2022

derekcollison Dec 12, 2022

ripienaar Dec 12, 2022

Jarema Dec 13, 2022

derekcollison Dec 12, 2022

piotrpio Dec 12, 2022

derekcollison Dec 12, 2022

ripienaar Dec 12, 2022

aricart Dec 13, 2022

derekcollison commented Dec 12, 2022

wallyqs Dec 12, 2022

piotrpio Dec 14, 2022

wallyqs Dec 12, 2022

piotrpio Dec 14, 2022

piotrpio commented Dec 13, 2022

wallyqs commented Dec 13, 2022 •

edited

piotrpio commented Dec 13, 2022

derekcollison commented Dec 13, 2022

wallyqs commented Dec 14, 2022

piotrpio commented Dec 14, 2022

wallyqs Dec 14, 2022

piotrpio Dec 19, 2022

wallyqs Dec 14, 2022

piotrpio Dec 19, 2022

Jarema left a comment

[ADDED] Service api improvements #1160

[ADDED] Service api improvements #1160

Conversation

piotrpio commented Dec 12, 2022 • edited

coveralls commented Dec 12, 2022 • edited

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

wallyqs Dec 12, 2022 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekcollison commented Dec 12, 2022

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

piotrpio commented Dec 13, 2022

wallyqs commented Dec 13, 2022 • edited

piotrpio commented Dec 13, 2022

derekcollison commented Dec 13, 2022

wallyqs commented Dec 14, 2022

piotrpio commented Dec 14, 2022

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

Jarema left a comment

Choose a reason for hiding this comment

piotrpio commented Dec 12, 2022 •

edited

coveralls commented Dec 12, 2022 •

edited

wallyqs Dec 12, 2022 •

edited

wallyqs commented Dec 13, 2022 •

edited