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

Feature/161 subsecond decimals #172

Merged
merged 9 commits into from
Mar 26, 2024
Merged

Conversation

EEParker
Copy link
Collaborator

@EEParker EEParker commented Mar 16, 2024

The first commit adds a sub-second decimal option to address #161.

The second commit adds a flag for disabling RenderedMessage per #167.

These both seem optional, but could be nice to have. I've included them here for review.

@EEParker EEParker requested a review from VictorioBerra March 16, 2024 03:54
@EEParker EEParker force-pushed the feature/161-subsecond-decimals branch 2 times, most recently from f0dd19e to 734392e Compare March 16, 2024 04:06
@EEParker
Copy link
Collaborator Author

@VictorioBerra If you have time I would like your opinion/review on these changes. Thank you

@VictorioBerra
Copy link
Member

Looks like the link to the docs is dead: http://dev.splunk.com/view/SP-CAAAE6P

Is there a more up to date link describing the tiemstamp format and the precision?

@EEParker
Copy link
Collaborator Author

https://docs.splunk.com/Documentation/Splunk/9.2.0/SearchReference/Commontimeformatvariables it looks like 3, 6 or 9 decimals are supported, so the PR probably needs to be updated to use an enum for millisecond, microseconds or nanoseconds.

@VictorioBerra
Copy link
Member

VictorioBerra commented Mar 16, 2024 via email

@EEParker
Copy link
Collaborator Author

I've updated this PR with the enum, added unit tests and updated documentation links.

@EEParker EEParker marked this pull request as ready for review March 16, 2024 22:07
@EEParker
Copy link
Collaborator Author

I've updated this PR with the enum, added unit tests and updated documentation links.

@@ -174,8 +182,11 @@ public void Format(LogEvent logEvent, TextWriter output)
JsonValueFormatter.WriteQuotedJsonString(logEvent.MessageTemplate.Text, output);
}

output.Write(",\"RenderedMessage\":");
JsonValueFormatter.WriteQuotedJsonString(logEvent.RenderMessage(_formatProvider), output);
if (!_renderMessage)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this logic be the other way round? ie if (_renderMessage) to match the _renderTemplate logic above?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right. I removed the !

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the fix

Copy link

@MatthewHays MatthewHays left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comment as to whether _renderMessage=true means to enable or disable the field from being sent to splunk, to match the meaning for _renderTemplate=true

@@ -182,7 +182,7 @@ public void Format(LogEvent logEvent, TextWriter output)
JsonValueFormatter.WriteQuotedJsonString(logEvent.MessageTemplate.Text, output);
}

if (!_renderMessage)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@EEParker
Copy link
Collaborator Author

EEParker commented Mar 25, 2024

@VictorioBerra I added the test code and splunk props.conf to make this work in the sample project.

I've also verified that microseconds (6) work, but I couldn't get Splunk ingestion to work with nanoseconds (9). I believe this is on the Splunk side but I'm not sure what is needed.

image

- add new tests
- add a docker-compose vscode project
- allow subsecond precision.
@EEParker EEParker force-pushed the feature/161-subsecond-decimals branch from 677f328 to b335663 Compare March 25, 2024 18:29
@VictorioBerra
Copy link
Member

Thanks for doing that, @EEParker can you share the changes you made to the props.conf? Maybe I could post on the Splunk forums about this?

@EEParker
Copy link
Collaborator Author

EEParker commented Mar 26, 2024

Thanks for doing that, @EEParker can you share the changes you made to the props.conf? Maybe I could post on the Splunk forums about this?

props.conf

TIME_PREFIX = \"time\"\:\s*\"
TIME_FORMAT = %Y-%m-%dT%H:%M:%S.%9N%Z

https://github.com/serilog-contrib/serilog-sinks-splunk/pull/172/files#diff-7bdfdc66d9929e53da33186cc68b1ccf09bb584d6b50ce4c0be241b259420395R1-R2

@EEParker
Copy link
Collaborator Author

Are you comfortable with merging this? I think a follow-up PR can be made to address any splunk samples that need to be changed. I would like to merge this in before #174.

@VictorioBerra
Copy link
Member

@EEParker Yeah id say go ahead, I like the addition of the .editoconfig and the docker additions.

@EEParker EEParker merged commit 87e782c into dev Mar 26, 2024
@EEParker EEParker deleted the feature/161-subsecond-decimals branch March 27, 2024 16:40
@EEParker EEParker added the Semver-Minor Minor Update - non-breaking changes label Apr 3, 2024
@EEParker EEParker added this to the 4.0 milestone Apr 3, 2024
EEParker added a commit that referenced this pull request Apr 3, 2024
* Bump System.Net.Http from 4.3.3 to 4.3.4 in /sample/Sample

Bumps System.Net.Http from 4.3.3 to 4.3.4.

Signed-off-by: dependabot[bot] <support@github.com>

* Bump System.Net.Http from 4.3.0 to 4.3.4 in /src/Serilog.Sinks.Splunk

Bumps System.Net.Http from 4.3.0 to 4.3.4.

Signed-off-by: dependabot[bot] <support@github.com>

* Bump System.Net.Http in /test/Serilog.Sinks.Splunk.Tests

Bumps System.Net.Http from 4.3.3 to 4.3.4.

Signed-off-by: dependabot[bot] <support@github.com>

* Bump System.Net.Http from 4.3.0 to 4.3.4 in /src/Serilog.Sinks.UDP

Bumps System.Net.Http from 4.3.0 to 4.3.4.

Signed-off-by: dependabot[bot] <support@github.com>

* add default batch config

* Update expired GitHub Key

* add default batch config

* Update expired GitHub Key

* fix bug dispose (#153)

* Regenerate API Key for NuGet

* Splunk 9.1 + .NET 6 (#166)

* Sample: Target net6.0. Added additional logging statements.

* Docker: Splunk 9.1.

* Tests: Target net6.0 and updated nuget packages.

* Added constant for services/collector. Event request trims / from uri. Updated collector URL for Splunk 9.1

* Update to serilog periodic batching 4.x (#168)

* Updated dependencies to Serilog 3.1 and Periodic Batching Sink to 4.0.0

* Update version to 4.0.0 due to the breaking constructor changes on the EventCollectorSink

* Update Dockerfile with net6.0 versions

* Use the latest version of Splunk

The 7.2 label does not appear to be available anymore.

* Add in missing build for net6.0

* Use forward slash for ignore

This should work on Windows as well

* Use the same Framework reference for the sample

Everything else is using at least net6.0 so the Sample should as well

---------

Co-authored-by: Justin Pfifer <justin.pfifer@carvana.com>
Co-authored-by: Jeff Parker, PE <EEParker@users.noreply.github.com>

* dotnet 8.0 updates (#171)

* Feature/161 subsecond decimals (#172)

* Fixes #161 add subsecond decimal configuration

* Fixes #167 Add a flag for RenderedMessage

* Update SubSecondPrecision to use an enum.

Based on 3,6,9 decimals, see
https://docs.splunk.com/Documentation/Splunk/9.2.0/SearchReference/Commontimeformatvariables

* Add unit tests and update documentation

* Invert renderMessage check

* Update sample docker-compose project

- add new tests
- add a docker-compose vscode project
- allow subsecond precision.

* Add comment for configuring splunk

* Fix formatting for nanoseconds

* Enable all tests in sample app

---------

Co-authored-by: Victorio Berra <toryberra@gmail.com>

* Set up GitHub Actions (#174)

* Enable GitHub Actions

* Main branch is called master in this repo

* Ignore .idea

* No need to offset builds by 200

* work on build for new project format

* add automatic release notes

---------

Co-authored-by: EEParker <EEParker@users.noreply.github.com>

* Filter branches that push to nuget (#177)

* Security Policy, scanning and dependency updates (#176)

* Add dependabot configuration

* Add codeql

* Add security policy

* Update dependabot.yml add newline

* update branches

* Update SECURITY.md

* Readme and package license fix

* Update dotnet pack command line

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Matthew Erbs <matthew.erbs@merbla.com>
Co-authored-by: Mohsen Rajabi <m.kabir8895@gmail.com>
Co-authored-by: Havagan <Havagan@users.noreply.github.com>
Co-authored-by: Justin Pfifer <61801015+jpfifer@users.noreply.github.com>
Co-authored-by: Justin Pfifer <justin.pfifer@carvana.com>
Co-authored-by: Victorio Berra <toryberra@gmail.com>
Co-authored-by: Nicholas Blumhardt <nblumhardt@nblumhardt.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Semver-Minor Minor Update - non-breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants