Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Go kit 0.4.0 release #427

Closed
6 tasks done
peterbourgon opened this issue Jan 10, 2017 · 26 comments
Closed
6 tasks done

Go kit 0.4.0 release #427

peterbourgon opened this issue Jan 10, 2017 · 26 comments

Comments

@peterbourgon
Copy link
Member

peterbourgon commented Jan 10, 2017 •
edited by ChrisHines
Loading

Edit: Due to lots of feature churn in the last few weeks/months, I've been convinced that it makes more sense to hold off on a 1.0.0 until things settle. Notes in comments below.

—

Hello! I want to cut a v1.0.0 release in the near future. This issue is created per the request of @ChrisHines to serve as a notice to anyone who may be interested, for example to raise questions, concerns, or propose specific additions.

The only thing outstanding that I'd like to get in is a promotion of log/experimental_level to log/level. I've asked @seh to weigh in on that, as he's previously commented on the package. Otherwise, let me know your thoughts. I'll leave this open until at least the weekend, so another 5 days.

edit: outstanding issues I'd want to resolve before a 1.0.0

If anyone feels strongly about either of those two Optionals, (cc: @willfaught) please get your PRs in within the next few days...
@ghost
Copy link

ghost commented Jan 10, 2017 via email •
edited by ghost
Loading

@peterbourgon
Copy link
Member Author

What is a connector architecture?

@ChrisHines
Copy link
Member

What will be the compatibility policy after v1.0.0 has been released?

@ghost
Copy link

ghost commented Jan 11, 2017

Connectors would be packages that implement the endpoint, transport, security, and other policies required by calling or called SaaS services like Salesforce.com, Workday, ServiceNow, LinkedIn, or Marketo. The connector architecture describes the wrapping of a SaaS service to call them or the wrapping of the go-kit service to be called by a SaaS services.

@peterbourgon
Copy link
Member Author

@pruggera No, nothing like this is planned.

@ChrisHines I plan to follow typical SemVer rules. Do you think that is sufficient?

@ChrisHines
Copy link
Member

I plan to follow typical SemVer rules. Do you think that is sufficient?

That's good. We shall see if it is sufficient. ;) Do we want to document this anywhere?

@seh
Copy link
Contributor

seh commented Jan 20, 2017

Please see yet another leveled logging proposal in #444.

@willfaught
Copy link
Contributor

Is there remaining work that needs to happen for the context or level tasks?

I have a couple questions about the experimental level package:

  • Isn't it possible to unintentionally filter out loggings with keys that aren't actually coming from the level package? For example, mylogger.Log("error", err) where mylogger is from levels.New and is configured to ignore the error level. The person setting the log level doesn't necessarily have visibility into what keys code might be using for Log, and vice versa. Should the levels be typed with a string alias like for context keys to prevent conflicts?
  • Why not use functional options for level.New instead of Config?

@peterbourgon
Copy link
Member Author

Is there remaining work that needs to happen for the context or level tasks?

Yes, I need to get around to fixing #421, and I'm waiting for a response on #444.

Isn't it possible to unintentionally filter out loggings with keys that aren't actually coming from the level package? For example, mylogger.Log("error", err) where mylogger is from levels.New and is configured to ignore the error level. The person setting the log level doesn't necessarily have visibility into what keys code might be using for Log, and vice versa. Should the levels be typed with a string alias like for context keys to prevent conflicts?

Yes, but not with that example. You'd need to invoke it like logger.Log("level", "error", "oops", "this may be ignored"). But given that we've made the executive decision early on that levels are nothing more than key=val pairs, then I see logger.Log(..., "level", "error", ...) as a pretty explicit declaration of intent. With that said, I'm open to more strictly typing the level key, and introspecting on that type via reflection. I'd accept a PR :)

Why not use functional options for level.New instead of Config?

There's no practical difference, but I can in hindsight see the value of using functional options for consistency with the rest of package log. I'd accept a PR :)

groob added a commit to groob/kit that referenced this issue Feb 9, 2017
@groob
Use variadic function to pass parameters instead of Config struct
ae92291 
Using function to pass optional params makes the API more consistent
with the rest of go-kit packages.

For go-kit#427
@groob groob mentioned this issue Feb 9, 2017
Merged
@peterbourgon
Copy link
Member Author

Update: 1.0.0 is just pending #470. ETA next week?

@ChrisHines
Copy link
Member

I should be able to finish up #470 this week.

@ChrisHines
Copy link
Member

#470 is merged. We still need to bring #449 up to date and merge it.

@ChrisHines
Copy link
Member

I've also added evaluating #474 to the check list.

@basvanbeek
Copy link
Member

I will add ClientResponseFunc handling in the gRPC transport so we can pull in metadata sent back with a server response (think upstream baggage)

Also will add the ability to call BeforeFuncs/AfterFuncs multiple times to append in a Client request. (We already have done this for the Servers)

@yurishkuro
Copy link
Contributor

I would like #403 and #463 addressed as they would change the discovery API. For us the metrics and discovery abstractions are the two primary reasons for using go-kit in Jaeger.

@peterbourgon
Copy link
Member Author

peterbourgon commented Mar 4, 2017 •
edited
Loading

We're already 2 months late on 1.0.0, changes to package sd will not be included. But I have no problem cutting a 1.1.0 or 2.0.0 very soon afterwards.

@willfaught
Copy link
Contributor

willfaught commented Mar 4, 2017 via email

@peterbourgon
Copy link
Member Author

Because I have made several intonations over the past several months that I would cut a 1.0.0 around the turn of the new year.

@willfaught
Copy link
Contributor

willfaught commented Mar 4, 2017 via email

@seh
Copy link
Contributor

seh commented Mar 4, 2017

I took the push for a version 1 to mean, "If you're thinking about changing any public interfaces, do it now and get it over with, because you won't get the chance to do so for a while afterward. Prioritize pending interface changes over other would-be contributions."

@willfaught
Copy link
Contributor

willfaught commented Mar 4, 2017 via email

@seh
Copy link
Contributor

seh commented Mar 4, 2017

The leveled logger was just added. Is a week or two long enough to say it's stable?

As the author of three proposals (none of which made it in) for that leveled logger, having seen how much of the design space we explored and discarded, I do expect the interface is stable. There is room for further optimization, but the exported interface is the one that @ChrisHines and @peterbourgon want.

@peterbourgon
Copy link
Member Author

After a night of reflection I agree that the feature churn is currently too high to justify a 1.0.0. I guess this is a good problem to have :)

So, I'll bundle up the current master + #474 into a 0.4.0 release. I want to wait for #474 because I think it will be a straightforward, boilerplate-reducing change. It will also bring API described in the logging proposal in line with what we have here.

Then, if @yurishkuro's work in package sd goes well, we can cut a 0.5.0 with that, as soon as it's stable. Letting that bake until mid-year and cutting a 1.0.0 for (say) GopherCon.

Thanks for the input, everyone.

@peterbourgon peterbourgon changed the title Go kit 1.0.0 release Go kit 0.4.0 release Mar 5, 2017
@ChrisHines
Copy link
Member

#474 is merged.

We should consider updating the package docs for log/level to remove the experimental disclaimer before tagging.

@peterbourgon
Copy link
Member Author

We should consider updating the package docs for log/level to remove the experimental disclaimer before tagging.

Oops, that was an oversight. Thanks for the spot. Fixed in master.

@peterbourgon
Copy link
Member Author

OK, great. I'll cut 0.4.0 tomorrow.

@ereOn ereOn mentioned this issue Mar 7, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@willfaught @peterbourgon @seh @ChrisHines @yurishkuro @basvanbeek