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

appender: Add offset time setting #2221

Closed
wants to merge 1 commit into from
Closed

appender: Add offset time setting #2221

wants to merge 1 commit into from

Conversation

hubertshelley
Copy link

Motivation

Solution

@hubertshelley hubertshelley requested a review from a team as a code owner July 15, 2022 07:56
@davidbarsky
Copy link
Member

The downside of this approach is that the use of a hard-coded offset does not handle daylight savings. I feel that if we're to support non-UTC timestamps inside of tracing-appender, we should use LocalTime to handle this correctly. While that is more expensive call than a UTF offset, it will likely be dwarfed by the file operations happening inside of tracing-appender. What do you think?

@hubertshelley
Copy link
Author

The downside of this approach is that the use of a hard-coded offset does not handle daylight savings. I feel that if we're to support non-UTC timestamps inside of tracing-appender, we should use LocalTime to handle this correctly. While that is more expensive call than a UTF offset, it will likely be dwarfed by the file operations happening inside of tracing-appender. What do you think?

我认为，具体偏移时间应该由编码者自主控制，是否需要将偏移时间编写的更灵活一些应该由编码者决定，我们可以提供设置方法。当然这只是我个人的想法。

hawkw added a commit that referenced this pull request Jul 18, 2022
@hawkw
appender: add a builder for constructing RollingFileAppenders
c5af7cc 
Several currently open PRs, such as #2225 and #2221, add new
configuration parameters to the rolling file appender in
`tracing-appender`. The best way to add new optional configuration
settings without breaking existing APIs or creating a very large number
of new constructors is to add a builder interface.

Since a number of PRs would all need to add the builder API, introducing
potential conflicts, this branch _just_ adds the builder interface
without adding any new configuration options. Once this merges, the
existing in-flight PRs can be rebased onto this branch to use the
builder interface without conflicting with each other.

Also, the `Builder::build` method is fallible and returns a `Result`,
rather than panicking. This is a relatively common pattern in Rust ---
for example, `std::thread::Builder::spawn` returns a `Result` if a new
thread cannot be spawned, while `std::thread::spawn` simply panics. This
allows users to handle appender initialization errors gracefully without
breaking the API of the existing `new` constructor.

Fixes #1953
@hawkw hawkw mentioned this pull request Jul 18, 2022
Merged
hawkw added a commit that referenced this pull request Jul 18, 2022
@hawkw
appender: add a builder for constructing RollingFileAppenders (#2227)
ab36c17 
## Motivation

Several currently open PRs, such as #2225 and #2221, add new
configuration parameters to the rolling file appender in
`tracing-appender`. The best way to add new optional configuration
settings without breaking existing APIs or creating a very large number
of new constructors is to add a builder interface.

## Solution

Since a number of PRs would all need to add the builder API, introducing
potential conflicts, this branch _just_ adds the builder interface
without adding any new configuration options. Once this merges, the
existing in-flight PRs can be rebased onto this branch to use the
builder interface without conflicting with each other.

Also, the `Builder::build` method is fallible and returns a `Result`,
rather than panicking. This is a relatively common pattern in Rust ---
for example, `std::thread::Builder::spawn` returns a `Result` if a new
thread cannot be spawned, while `std::thread::spawn` simply panics. This
allows users to handle appender initialization errors gracefully without
breaking the API of the existing `new` constructor.

Fixes #1953

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Jul 20, 2022
@hawkw
appender: add a builder for constructing RollingFileAppenders (#2227)
7f70dc6 
## Motivation

Several currently open PRs, such as #2225 and #2221, add new
configuration parameters to the rolling file appender in
`tracing-appender`. The best way to add new optional configuration
settings without breaking existing APIs or creating a very large number
of new constructors is to add a builder interface.

## Solution

Since a number of PRs would all need to add the builder API, introducing
potential conflicts, this branch _just_ adds the builder interface
without adding any new configuration options. Once this merges, the
existing in-flight PRs can be rebased onto this branch to use the
builder interface without conflicting with each other.

Also, the `Builder::build` method is fallible and returns a `Result`,
rather than panicking. This is a relatively common pattern in Rust ---
for example, `std::thread::Builder::spawn` returns a `Result` if a new
thread cannot be spawned, while `std::thread::spawn` simply panics. This
allows users to handle appender initialization errors gracefully without
breaking the API of the existing `new` constructor.

Fixes #1953

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Jul 20, 2022
@hawkw
appender: add a builder for constructing RollingFileAppenders (#2227)
67b9474 
## Motivation

Several currently open PRs, such as #2225 and #2221, add new
configuration parameters to the rolling file appender in
`tracing-appender`. The best way to add new optional configuration
settings without breaking existing APIs or creating a very large number
of new constructors is to add a builder interface.

## Solution

Since a number of PRs would all need to add the builder API, introducing
potential conflicts, this branch _just_ adds the builder interface
without adding any new configuration options. Once this merges, the
existing in-flight PRs can be rebased onto this branch to use the
builder interface without conflicting with each other.

Also, the `Builder::build` method is fallible and returns a `Result`,
rather than panicking. This is a relatively common pattern in Rust ---
for example, `std::thread::Builder::spawn` returns a `Result` if a new
thread cannot be spawned, while `std::thread::spawn` simply panics. This
allows users to handle appender initialization errors gracefully without
breaking the API of the existing `new` constructor.

Fixes #1953

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Jul 20, 2022
@hawkw
appender: add a builder for constructing RollingFileAppenders (#2227)
28a0c99 
## Motivation

Several currently open PRs, such as #2225 and #2221, add new
configuration parameters to the rolling file appender in
`tracing-appender`. The best way to add new optional configuration
settings without breaking existing APIs or creating a very large number
of new constructors is to add a builder interface.

## Solution

Since a number of PRs would all need to add the builder API, introducing
potential conflicts, this branch _just_ adds the builder interface
without adding any new configuration options. Once this merges, the
existing in-flight PRs can be rebased onto this branch to use the
builder interface without conflicting with each other.

Also, the `Builder::build` method is fallible and returns a `Result`,
rather than panicking. This is a relatively common pattern in Rust ---
for example, `std::thread::Builder::spawn` returns a `Result` if a new
thread cannot be spawned, while `std::thread::spawn` simply panics. This
allows users to handle appender initialization errors gracefully without
breaking the API of the existing `new` constructor.

Fixes #1953

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hubertshelley hubertshelley closed this by deleting the head repository Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hubertshelley @davidbarsky