Skip to content
This repository has been archived by the owner on May 25, 2022. It is now read-only.

Update Changelog ahead of major set of breaking changes #430

Merged
merged 3 commits into from
Mar 28, 2022

Conversation

djaglowski
Copy link
Member

@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #430 (6341975) into main (a8ec8d4) will decrease coverage by 0.0%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #430     +/-   ##
=======================================
- Coverage   75.4%   75.3%   -0.1%     
=======================================
  Files         83      83             
  Lines       3888    3888             
=======================================
- Hits        2934    2931      -3     
- Misses       671     674      +3     
  Partials     283     283             
Impacted Files Coverage Δ
operator/transformer/recombine/recombine.go 75.6% <0.0%> (-2.1%) ⬇️

CHANGELOG.md Outdated
@@ -5,13 +5,41 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).


## [0.26.1] - 2022-03-10
## [0.90.0] - 2022-03-14
Copy link
Member Author

Choose a reason for hiding this comment

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

The specified version and date are placeholders.

I think these changes may warrant a more than a minor version bump, to indicate the significance.

@djaglowski
Copy link
Member Author

djaglowski commented Mar 10, 2022

I believe these changes represent all of the breaking changes that have been agreed upon. However, the overall effort to align this library with the logs data model will yet include some additive changes, such as enhancements to field syntax.

Additionally, #431 would be a notable breaking change but has not yet been agreed upon. Implementation of that depends on the enhancements to field syntax, and so may take a couple weeks. I'm inclined to cut this release sooner and if necessary a second breaking release in a couple weeks.

cc: @tigrannajaryan @jsirianni @pmm-sumo @dmitryax

@djaglowski djaglowski marked this pull request as ready for review March 10, 2022 18:27
@djaglowski djaglowski requested a review from a team March 10, 2022 18:27
@dmitryax
Copy link
Member

dmitryax commented Mar 10, 2022

Do we have a chance to make all the changes (or at least some of them) in a backward compatible way whenever it's feasible?

For example, can #364 support both $body and body for now, throwing warning if it's configured with $?

Sorry for bringing it too late.

@dmitryax
Copy link
Member

Also it seems like we are accumulating all of them for one release. What is the reason for that? As a user, it would rather prefer adopting smaller amount of changes between upgrades rather than all at once, but others may not agree with me.

@djaglowski
Copy link
Member Author

Also it seems like we are accumulating all of them for one release. What is the reason for that? As a user, it would rather prefer adopting smaller amount of changes between upgrades rather than all at once, but others may not agree with me.

This strategy was agreed in the Log SIG several weeks ago. I see tradeoffs either way, but I think this is the right approach. The risk with several consecutive breaking changes is that users have to keep revisiting and redeploying, and I think many would conclude at some point that it's not worth keeping up. At least with one, they have one painful experience and hopefully don't have to keep thinking about it. Besides, I think at this point it would introduce quite some delays to spread them out over several releases (of the collector).

@djaglowski
Copy link
Member Author

Do we have a chance to make all the changes (or at least some of them) in a backward compatible way whenever it's feasible?

For example, can #364 support both $body and body for now, throwing warning if it's configured with $?

Sorry for bringing it too late.

Yeah, I think that one seems reasonable. I'll confirm and update it unless I'm forgetting a reason why it can't.

I'm not sure any of the others really can be made backwards compatible. I suppose technically #372 can add config flags and deprecate the old ones, but I think it's not worth it in that case.

@jsirianni
Copy link
Member

I think it would be nice to write a quick doc somewhere showing how to update existing configs. I would be happy to help with this, I am not sure where this doc would live. Perhaps the changelog / release.

Example:

  • $body --> body
  • how to handle entry.Timestamp now that it does not have a default
  • restructure --> add / move / etc
  • etc

@djaglowski
Copy link
Member Author

@jsirianni, that's a good idea. I tried to be verbose in the changelog, but something more end-user oriented would be great. If you want to rough out how you think that info would be best presented, I'm happy to help fill in the details. I think the best place for this info would be on the release page.

@djaglowski
Copy link
Member Author

I've updated #364 to allow the former syntax as well. Unfortunately, there's not a great way to complain about the deprecated usage because it is processed during unmarshaling, where we don't yet have a handle to a logger.

@djaglowski djaglowski changed the title Update Changelong ahead of major set of breaking changes Update Changelog ahead of major set of breaking changes Mar 15, 2022
@djaglowski
Copy link
Member Author

djaglowski commented Mar 18, 2022

@jsirianni, here is my draft for a special section at the top of the release notes. PTAL

Upgrading to version 0.28.0

Several changes have been made that affect configuration for the filelog, syslog, tcplog, udplog, and journald receivers.

Update all usages of field syntax

Field syntax no longer requires the $ character. Instead each field must begin with body, attributes, or resource.

Deprecated ExampleUpdated Equivalent

$attributes["log.file.name"]

attributes["log.file.name"]

$resource["host.name"]

resource["host.name"]

$body.foo

body.foo

$.foo

body.foo

foo

body.foo

Replace usages of restructure operator

The restructure operator has been removed. Use add, copy, flatten, move, remove, and retain operators instead.

Deprecated ExampleUpdated Equivalent
operators:
  - type: restructure
    ops:
      - add:
        field: set_me
        value: foo
      - add:
        field: overwrite_me
        value: bar
      - move:
        from: details.env
        to: env
      - remove:
        field: delete_me
operators:
  - type: add
    field: body.set_me
    value: foo
  - type: add
    field: body.overwrite_me
    value: bar
  - type: move
    from: body.details.env
    to: body.env
  - type: remove
    field: body.delete_me

Replace usages of metadata operator

The metadata operator has been removed. Use add, copy, or move operators instead.

Deprecated ExampleUpdated Equivalent
operators:
  - type: metadata
    attributes:
      environment: production
      file: 'EXPR( $body.file )'
    resource:
      cluster: blue
operators:
  - type: add
    field: attributes.environment
    value: production
  - type: copy
    from: body.file
    to: attributes.file
  - type: add
    field: resource.cluster
    value: blue
  - type: move
    from: body.foo
    to: attributes.bar

Update filelog attribute references

The filelog receiver has adopted slightly different attribute names in order to match newly established semantic conventions. Configurations that previously refered to the file.* attributes should be updated.

Deprecated AttributeUpdated Equivalent

file.name

log.file.name

file.path

log.file.path

file.name.resolved

log.file.name_resolved

file.path.resolved

log.file.path_resolved

Note to Vendors: A log record's Timestamp field may now be nil

A recent change to the Log Data Model has redefined the usage of the Timestamp field. Correspondingly, this field is no longer initialized by default. All Log Exporters should be evaluated to ensure this change is handled accordingly.

Log exporters can use the following logic to mimic the previous funcationality (psuedocode):

timestamp := log.ObservedTimestamp
if log.Timestamp != nil {
  timestamp = log.Timestamp
}

@jsirianni
Copy link
Member

@djaglowski looks great, exactly what I had in mind.

@djaglowski
Copy link
Member Author

@open-telemetry/log-collection-approvers, PTAL. Any additional feedback on this proposal? Can we move forward?

I would like to merge these PRs starting Wednesday if there are no objections.

@tigrannajaryan
Copy link
Member

attributes.["log.file.name"]

Is this the actual syntax or there is an extra dot after attributes that should not be there, i.e. it should be attributes["log.file.name"]?

@djaglowski
Copy link
Member Author

Good catch @tigrannajaryan. Fixed.

@tigrannajaryan
Copy link
Member

LGTM.

@dmitryax you had concerns about this change, PTAL.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM, but I would like to also see @dmitryax approval.

@djaglowski
Copy link
Member Author

djaglowski commented Mar 23, 2022

Discussed in Log SIG today. Will use a normal minor version bump for this release.

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM

@djaglowski
Copy link
Member Author

All prerequisite PRs are now merged.

Before this release is cut, I would like to call attention to the one remaining proposal that would be a breaking change. If #431 is accepted, it will ultimately require some users to perform a second update to their configurations. In many cases this will apply to lines that have just been updated due to this release. If this churn is coming and can be avoided, I think it is worth a slight delay in this release.

# Pre-v0.28.0 Config
parse_from: sev

# Post-v0.28.0 Config
parse_from: body.sev

# Config if #431 is accepted
parse_from: attributes.sev

At this point, being late in the week, I'd like to release on Monday so I can be more available to support upgrading users.

In the meantime, I would appreciate any additional thoughts on #431. I will work ahead on the implementation in case consensus can be reached quickly. We can update/validate the release timeline on Monday.

cc: @tigrannajaryan, @dmitryax, @jsirianni, @pmm-sumo

@djaglowski
Copy link
Member Author

I will cut this release this afternoon.

@djaglowski djaglowski merged commit f9887c7 into open-telemetry:main Mar 28, 2022
@djaglowski djaglowski deleted the changelog-breaking branch March 28, 2022 15:20
jsirianni pushed a commit to jsirianni/opentelemetry-log-collection that referenced this pull request Mar 28, 2022
…ry#430)

* Add changelog entry for major set of breaking changes.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants