Skip to content

Add configuration to allow transformation of durations to integers.#5416

Merged
istio-testing merged 1 commit intoistio:masterfrom
jeffmendoza:fluentd-duration
Jul 3, 2018
Merged

Add configuration to allow transformation of durations to integers.#5416
istio-testing merged 1 commit intoistio:masterfrom
jeffmendoza:fluentd-duration

Conversation

@jeffmendoza
Copy link
Copy Markdown
Contributor

The fluentd handler currently transforms durations to their string
representation for log messages. This commit adds a configuration to
override this default behaviour and tranform durations to integers of
unit millisecond.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this should be an enum instead.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

enum would be more flexible, but I'm not quite sure what the other values would be.

should we have integer_duration be the default (and name this something like: durations_as_string) ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't mind switching the default, if that makes the most sense.

@codecov
Copy link
Copy Markdown

codecov bot commented May 4, 2018

Codecov Report

Merging #5416 into master will increase coverage by 1%.
The diff coverage is 50%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #5416   +/-   ##
======================================
+ Coverage      71%     71%   +1%     
======================================
  Files         367     367           
  Lines       31572   31620   +48     
======================================
+ Hits        22340   22394   +54     
+ Misses       8342    8329   -13     
- Partials      890     897    +7
Impacted Files Coverage Δ
mixer/adapter/fluentd/fluentd.go 74% <50%> (-1%) ⬇️
mixer/adapter/solarwinds/metrics_handler.go 84% <0%> (-9%) ⬇️
mixer/adapter/rbac/rbacStore.go 76% <0%> (-3%) ⬇️
mixer/adapter/statsd/statsd.go 93% <0%> (-2%) ⬇️
mixer/adapter/servicecontrol/utils.go 88% <0%> (-1%) ⬇️
istioctl/cmd/istioctl/proxyconfig.go 13% <0%> (-1%) ⬇️
mixer/pkg/protobuf/yaml/resolver.go 59% <0%> (-1%) ⬇️
istioctl/cmd/istioctl/main.go 43% <0%> (-1%) ⬇️
mixer/adapter/list/ipList.go 100% <0%> (ø) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df49d93...c6018b3. Read the comment docs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why 'n' here ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

enum would be more flexible, but I'm not quite sure what the other values would be.

should we have integer_duration be the default (and name this something like: durations_as_string) ?

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label May 18, 2018
@stale
Copy link
Copy Markdown

stale bot commented Jun 22, 2018

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale label Jun 22, 2018
@douglas-reid
Copy link
Copy Markdown
Contributor

let's rebase and push

@stale stale bot removed the stale label Jun 25, 2018
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 25, 2018
@jeffmendoza
Copy link
Copy Markdown
Contributor Author

Rebased

@douglas-reid
Copy link
Copy Markdown
Contributor

/lgtm

I'd still prefer ints as the default behavior, but since this has been around for awhile as-is without complaint, I think this is fine.

@jeffmendoza
Copy link
Copy Markdown
Contributor Author

@douglas-reid Can you re-trigger the lint test? Thanks.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 29, 2018
@istio-testing istio-testing removed lgtm needs-rebase Indicates a PR needs to be rebased before being merged labels Jul 2, 2018
@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Jul 2, 2018

@jeffmendoza: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/e2e-bookInfoTests.sh 06917798b6852f9b77829d8909e29eef28d7d11e link /test e2e-bookInfo
prow/istio-pilot-e2e.sh 06917798b6852f9b77829d8909e29eef28d7d11e link /test istio-pilot-e2e
Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@jeffmendoza
Copy link
Copy Markdown
Contributor Author

/test istio-pilot-e2e-envoyv2-v1alpha3

The fluentd handler currently transforms durations to their string
representation for log messages. This commit adds a configuration to
override this default behaviour and tranform durations to integers of
unit millisecond.
@douglas-reid
Copy link
Copy Markdown
Contributor

/lgtm

@istio-testing
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: douglas-reid, jeffmendoza

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@istio-testing istio-testing merged commit 664bb4a into istio:master Jul 3, 2018
istio-testing pushed a commit that referenced this pull request Jul 7, 2018
* Introducing mcpc, a command-line  mcp client tool. (#6715)

* Set default requests cpu resource to all deployments (#6229)

* Set default requests cpu resource to all deployments

* Description added

* Requested CPU decreased to meet test env node capacity

* Setting requested cpu to 10m as most k8s platforms are defaulting to 0m

* Merge master remains

* Apply the global resources to the new gateways

* Updated goldens to cpu 10m

* Clean up and rename the global default resources

* Redundant Redis servers were removed + defer in a loop is not recommended. (#6768)

* Fix upgrade. (#6792)

* Fix upgrade.

* Format

* Move back the fail close - not clear if we have time to get mixer client in this build, we can remove in next

* Add configuration to allow transformation of durations to integers. (#5416)

The fluentd handler currently transforms durations to their string
representation for log messages. This commit adds a configuration to
override this default behaviour and tranform durations to integers of
unit millisecond.

* Add workload and service dashboard (#6789)

* Add workload and service dashboard

* fix typo

* Skip service dash e2e test for now

* Add rushed, missed workload dash

* Touch up

* Commenting out test until can replicate locally and fix

* simplify fmt script (#6798)

* simplify fmt script

- goimports is a superset of gofmt
- shouldn't need to remove spaces
- now utilizing -l flag to tell us if there is a diff

* always update gofmt to latest

* spacing that is now considered incorrect

* support hash based routing for soft session affinity (#6742)

* support sticky sessions

* fmt :(

* update servicediscovery fakes

* bump istio/api

* use updated api for ring hash

* support session affinity for sidecar

* update destination rule yaml to reflect new api

* ttl is a string

* unset sdsUdsPath in configmap (#6799)

* fix bug so that destination.service.** attributes are collected (#6801)

* remove unnecessary generated attributes finding. (#6785)

* modify docker template files for proxyv2 (#6790)

* Long-running testing improvements (#6800)

* Add values for config map settings, including access log.
More docs.

* Updates and improvements for the stress-testing configs.

* Add values for config map settings, including access log. (#6797)

* Add values for config map settings, including access log.
More docs.

* Updates and improvements for the stress-testing configs.

* Address review comments

* Merged wrong files

* Add the setup helm file - this change now depend on the previous PR.

* Sync with remote, remove accidentally added files.

* Another accidental file

* SNI routing from sidecar to gateway with virtual services (#6402)

* quick sni matching 1st pass with no refactoring of existing code

* use shriram's api sha

* quick pass at using tls block

* add some validation

* copyright

* fix lint + remove deadcode

* rename protocol tcp_tls -> tls

* update back to istio/api master

* remove accidentally added test file

* add tls block to gateway logic

* add todos

* basic sni wildcard implementation

* add tcp, fix problems with rbac, matching

* better tcp + tls validation

* address code review comments

* remove out of date comment

* update comments

* fix compile error

* use tcp proxy in tcp routing

* add tcp routing e2e test

* add forgotten vs config file + update description of test

* Comments, bug fixes

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* cleanup gateway tcp test

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* moving networking test yamls

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* tcp/tls tests

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* yaml fixes

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* fix file switcheroo

* port matches

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* fix authN plugin overwriting TLS context

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* more tests - route via egress gateway

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* yaml fixes

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* initialize prom variables

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* split tests

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* final test fix hopefully

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* revert gateway tweaks

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* Add myself back to OWNERS, sort the list alphabetically. (#6830)

* Upload cloudfoundry test result to GCS (#6829)

* Fix typos in command-line output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants