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.

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

Update AddHoneycomb with settings instance to use Action<> #29

Merged
merged 7 commits into from
Mar 26, 2021

Conversation

MikeGoldsmith
Copy link
Contributor

Fixes #17.

Updates the serviceCollection.AddHoneycomb programatic overload to take an Action<HoneycombApiSettings>.

@MikeGoldsmith MikeGoldsmith requested a review from a team March 8, 2021 14:52
@MikeGoldsmith MikeGoldsmith added status: review needed Changes need review. type: bug Something isn't working version: bump minor A PR that adds behavior, but is backwards-compatible. labels Mar 11, 2021
README.md Outdated Show resolved Hide resolved
@ericsampson
Copy link
Contributor

Out of curiosity, is there a precedence/convention of not supporting both options (class and Action) ? I've seen different libraries choose different combinations of IConfiguration, LibSettings, and Action<LibSettings>configureLibSettings

@MikeGoldsmith
Copy link
Contributor Author

Out of curiosity, is there a precedence/convention of not supporting both options (class and Action) ? I've seen different libraries choose different combinations of IConfiguration, LibSettings, and Action<LibSettings>configureLibSettings

There isn't a convention of preferring one type of parameter vs another. I personally find the Action<T> style the better solution because the library is in control of creating the settings instance which i handed to the function. I've used both a class instance and the Action<> together in the past, where the Action<T> just creates the settings instance and then passes into the other method.

Preetham Reddy and others added 6 commits March 24, 2021 11:31
Co-authored-by: Preetham Reddy <preetham@techfabric.com>
Fixes #16.

Updates the default SendFrequency to 10000ms (10 seconds) to match documentation and default value in example appsettings.json.

Co-authored-by: Paul Osman <paul@honeycomb.io>
Fixes #14.

Adds HoneycombApiSettings.WriteKey to replace TeamId to improve consistency between beeline implementations. TeamId is marked as deprecated and any usage manipulates the new property to ensure backwards compatibility. Docs and usage examples also updated.

Also includes replacing package references in Honeycomb.AspnetCore and the Sample projects with project references so it uses the local projects instead of depending on downloaded packages. This allows easier code debugging and for any changes in the root project to be reflected in sub-projects without having to release the main project first.
* main:
  Fix typo in project file. (#31)
  Add WriteKey and deprecate TeamId (#30)
  Update default SendFrequency to 10 seconds (#27)
  Stopping the timer before calculating the elapsedmilliseconds (#28)

# Conflicts:
#	README.md
@MikeGoldsmith MikeGoldsmith requested a review from vreynolds March 24, 2021 11:37
@ericsampson
Copy link
Contributor

@MikeGoldsmith what I noticing is that the way it was originally doesn't work, unless I'm being extra slow today?

Here's an example demonstrating how it was before, and what happens.
image

@MikeGoldsmith
Copy link
Contributor Author

@MikeGoldsmith what I noticing is that the way it was originally doesn't work, unless I'm being extra slow today?

Here's an example demonstrating how it was before, and what happens.
image

That's correct, the previous way of calling services.Configure(settings); didn't work. The updated version passing an Action<HoneycombApiSettings> to configure does work. I believe it's to do with scoping of the services DI graph, but not 100% certain. I've used it previously with an Action and it worked here too 🤷‍♂️ .

@MikeGoldsmith MikeGoldsmith merged commit b93ebe6 into main Mar 26, 2021
@MikeGoldsmith MikeGoldsmith deleted the mike/team-not-set branch March 26, 2021 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: review needed Changes need review. type: bug Something isn't working version: bump minor A PR that adds behavior, but is backwards-compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

X-Honeycomb-Team not set
3 participants