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

LogStreamName is not configurable #195

Closed
1 of 2 tasks
Ady88w1 opened this issue Apr 7, 2022 · 11 comments
Closed
1 of 2 tasks

LogStreamName is not configurable #195

Ady88w1 opened this issue Apr 7, 2022 · 11 comments
Labels
feature-request A feature should be added or improved. module/logging p2 This is a standard priority issue queued

Comments

@Ady88w1
Copy link

Ady88w1 commented Apr 7, 2022

Describe the feature

I'm migrating a custom logging solution using CloudWatch from the AWS SDK to NLog. This package seems to randomize the log stream name by placing a guid into the LogStream.

Use Case

Each time the executable restarts, a new log stream is created which looses the grouping and search ability you get from grouping logs into a common log stream.

Proposed Solution

Allow the LogStreamName name to be configurable, in the same way that a file name can be configurable when logging to a file.

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

AWS .NET SDK and/or Package version used

NLog 4.7

Targeted .NET Platform

.NET Standard

Operating System and version

Agnostic

@Ady88w1 Ady88w1 added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Apr 7, 2022
@ashishdhingra
Copy link
Contributor

Hi @Ady88w1,

Thanks for opening feature request. Please refer #1 (comment). The way CloudWatch Logs is designed is each process instance needs its own stream.

Thanks,
Ashish

@ashishdhingra ashishdhingra added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. module/logging and removed needs-triage This issue or PR still needs to be triaged. labels Apr 7, 2022
@Ady88w1
Copy link
Author

Ady88w1 commented Apr 7, 2022

Hi @Ady88w1,

Thanks for opening feature request. Please refer #1 (comment). The way CloudWatch Logs is designed is each process instance needs its own stream.

Thanks, Ashish

Hi @ashishdhingra,

I am aware of that, please read my original post. I am already using CloudWatch, I just wanted to migrate to NLog, or some other logging format. This library seems very opinionated about how to define log streams, which I don't really understand as it's not anywhere else, other than this.

For example, I run long processes on Spot instances, when the spot instance is shut down, and resumed, I want the task to continue using the log stream it was previously using.

e.g. LogStreamName:
/[uniquetaskid]

As the task is unique, there will only ever be one instance using the log stream.

You resume a Log Stream but calling DescribeLogStreams and getting the current uploadSequenceToken.

@Ady88w1
Copy link
Author

Ady88w1 commented Apr 7, 2022

It seems it's faster to simply create a PR: #196

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Apr 8, 2022
@Ady88w1
Copy link
Author

Ady88w1 commented Apr 16, 2022

@normj I'm really unsure how you can say there is no good way to make that happen?

for example (NLog):
<target name="aws" type="AWSTarget" logGroup="my/log" logStreamUniqueKey="${date:format=yyyy-MM-ddTHH.mm.ss.fff}" region="us-east-1" />

Would replicate the existing functionality, alternatively you could use any other Layout Renderer, such as guid.
<target name="aws" type="AWSTarget" logGroup="my/log" logStreamUniqueKey="${guid}" region="us-east-1" />

Considering this is supposed to be an AWS lead project, I'm unsure why you are so insistent to remove AWS CloudWatch features.

@normj
Copy link
Member

normj commented Apr 18, 2022

Okay so you can in NLog have a way of make logStreamUniqueKey work like the existing behavior. You might be able to come up with some schema in the other logging libraries but you know most users would just set the value to a static value.

I am protective against adding this feature because it will make the performance of the library unacceptable when the PutLogEvents is failing due to the sequence token out of date. If we don't make the best practice for using CloudWatch Logs the obvious pick the team will be hammered with GitHub issues about poor performance in prod.

This library is following the same pattern as every other AWS service that I can think of that integrates with CloudWatch Logs and has a log stream per process.

@Ady88w1
Copy link
Author

Ady88w1 commented Apr 20, 2022

@normj I understand your concern at this, but I think preventing a developer from using CloudWatch in the way they want to use it is not a good solution either (In my case logs of a task can be retrieved by the UI). Maybe describing the property in a way to discourage improper use would be the way forward?

For example, in React you cannot set HTML directly, any HTML in a string is added to a text node to prevent cross site scripting issues. However because there are legitimate reasons why you may want to set raw HTML, there is a property called dangerouslySetInnerHTML. The language of the property is clear that using the property is ill advised, and should only be set if you know what you are doing.

@ashishdhingra ashishdhingra added needs-review p2 This is a standard priority issue labels Dec 9, 2022
@ryanwilliams83
Copy link

I'm hoping to use the EC2 Instance Id as the LogStreamName but it doesn't appear that I have the option to configure it which is really frustrating it.

I can achieve the desired outcome by logging to disk and configuring the CloudWatchLogs agent to upload the logs to CloudWatch in the desired naming convention.

Surely we can get a thread safe implementation of the AWSLogger provider that allows people to nominate the LogStreamName (albeit with documentation that explains the importance of not having two computers or even two applications attempting to write to the same log stream concurrently).

@sander-bol
Copy link

Considering the recent changes to the Cloudwatch API, do the previous comments still hold true?

https://aws.amazon.com/about-aws/whats-new/2023/01/amazon-cloudwatch-logs-log-stream-transaction-quota-sequencetoken-requirement/

Amazon CloudWatch Logs is also removing the requirement of providing a sequence token when calling Amazon CloudWatch Logs PutLogEvents API. CloudWatch Logs will still accept PutLogEvents API request with sequence token and return a PutLogEvents API response with a sequence token to maintain backwards compatibility. Your PutLogEvents API calls will be accepted, and CloudWatch Logs won't return InvalidSequenceToken errors irrespective of providing an invalid sequence token. We expect this change to simplify your integration with CloudWatch Logs because you won't need to coordinate across different clients writing to the same log stream.

@normj
Copy link
Member

normj commented Mar 25, 2023

@sbol-coolblue I agree that since CloudWatch Logs has removed the requirement of sequence tokens we can add the feature to configure a specific stream. I'll try to get to this when I can. If anybody is feeling like submitting a PR to get this done quicker that would be great. Basically we need remove the sequence token logic and then add a new stream name config property. If the property name is set used that name otherwise use our existing logic for determining stream name.

@ashovlin
Copy link
Member

ashovlin commented Apr 9, 2024

We've just released a new minor version of each package, adding a LogStreamName setting which can be used to override the full log stream name. When this is set LogStreamNameSuffix and LogStreamNamePrefix will be ignored since we're no longer using the computed name.

This is possible now as @sander-bol and @normj discussed above - in 2023 CloudWatch Logs removed the SequenceToken requirement. This removes the need to either split log ingestion across multiple log streams via unique stream names (the technique that this library relied upon), or else to coordinate the sequence token across multiple producers.

We removed the sequence token handling entirely in this release (even when the new LogStreamName setting isn't used) since the documentation for PutLogEvents says it's now ignored.

Let us know if you see any issues with the new setting.

@ashovlin ashovlin added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Apr 9, 2024
@ashovlin ashovlin removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Apr 11, 2024
Copy link

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. module/logging p2 This is a standard priority issue queued
Projects
None yet
Development

No branches or pull requests

6 participants