Implement config validation for access logs, and implement use of descriptors by ZackButcher · Pull Request #500 · istio/old_mixer_repo

ZackButcher · 2017-03-31T21:09:28Z

We implement config validation. At the same time we update the access log manager to use descriptors. This update makes it natural to remove the log_format enum from the access log config: it adds substantial special logic if we 'know' about certain descriptors automagically, so I've removed the special casing around apache common/combined access logs. As a result, once again, access logs and application logs are essentially the same.

This change is

istio-testing · 2017-03-31T21:16:28Z

Jenkins job mixer/presubmit passed

istio-testing · 2017-03-31T21:17:10Z

Jenkins job mixer/presubmit passed

istio-testing · 2017-03-31T21:18:12Z

Jenkins job mixer/presubmit passed

istio-testing · 2017-03-31T21:22:58Z

Jenkins job istio/mixer-pr-master passed

istio-testing · 2017-03-31T21:25:04Z

Jenkins job istio/mixer-pr-master passed

codecov · 2017-03-31T21:47:26Z

Codecov Report

Merging #500 into master will increase coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #500      +/-   ##
=========================================
+ Coverage   88.32%   88.4%   +0.08%     
=========================================
  Files          56      56              
  Lines        3049    3053       +4     
=========================================
+ Hits         2693    2699       +6     
+ Misses        312     310       -2     
  Partials       44      44

Impacted Files	Coverage Δ
pkg/aspect/applicationLogsManager.go	`98.14% <100%> (+0.01%)`	⬆️
pkg/aspect/common.go	`100% <100%> (ø)`	⬆️
pkg/aspect/accessLogsManager.go	`100% <100%> (+6.15%)`	⬆️
pkg/config/manager.go	`97.05% <0%> (-2.95%)`	⬇️

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 bb04cef...01c75af. Read the comment docs.

istio-testing · 2017-03-31T21:57:58Z

Jenkins job mixer/presubmit passed

istio-testing · 2017-03-31T22:04:27Z

Jenkins job istio/mixer-pr-master passed

ZackButcher · 2017-03-31T22:21:06Z

This PR also implements validation on log template param expressions and wires it up in both access and application logs.

mandarjog · 2017-03-31T22:18:54Z

      logName: "access_log"
      log:
-        logFormat: 1 # COMMON
+        descriptorName: "common"


we have used descriptor_name and descriptorName let's pick one.

mandarjog · 2017-03-31T22:21:06Z

      logName: "access_log"
      log:
-        logFormat: 1 # COMMON
+        descriptorName: "common"


looking at the keys again, and how it plays out with UX, descriptor_name is really logType

descriptor_name is consistent with all the other aspects (including application logs). I think it'd be pretty odd to make this one aspect different.

mandarjog · 2017-03-31T22:23:40Z

+    expiration:
+      seconds: 1
+logs:
+  - name: common


Let's make the name accesslog.common and accesslog.combined
That we can retire the accessLog kind. Having descriptive names serves similar purpose.

ZackButcher · 2017-03-31T22:39:13Z

PTAL

istio-testing · 2017-03-31T22:40:11Z

Jenkins job mixer/presubmit passed

istio-testing · 2017-03-31T22:47:31Z

Jenkins job istio/mixer-pr-master passed

mandarjog · 2017-03-31T23:28:06Z

 		LogName: "combined_access_log",
 		Log: &aconfig.AccessLogsParams_AccessLog{
-			LogFormat: aconfig.COMBINED,
+			DescriptorName: "common",


Since common log and combined log are known log formats, can we add a test that ensures that our packaged default descriptors actually produce the correct log entries?

We should also agree on a mechanism to provide default descriptors. I think mixer config manager with packaged default set of descriptors as yml files. (like we have in testdata/ is a reasonable start)

I was chatting with Doug about this (and more generally how we've been going round and round on this) and we came to the same conclusion: we'll have to figure out some way to package an always-available istio-global-config with things like these descriptors, the common metric descriptors, and so on. I'm actually not opposed to it being static protos that we compile into the binary (speaking of going round-and-round).

istio-testing · 2017-04-03T22:31:45Z

Jenkins job mixer/presubmit passed

istio-testing · 2017-04-03T22:42:43Z

Jenkins job istio/mixer-pr-master passed

ZackButcher · 2017-04-03T22:42:57Z

PTAL.

istio-testing · 2017-04-04T22:02:51Z

Jenkins job mixer/presubmit passed

istio-testing · 2017-04-04T22:08:42Z

Jenkins job istio/mixer-pr-master passed

…t natural to remove the log_format enum from the access log config: it adds substantial special logic if we 'know' about certain descriptors automagically, so I've removed the special casing around apache common/combined access logs. As a result, once again, access logs and application logs are essentially the same.

…ly valid and that the attributes exist in the system to satisfy their expressions. Wire this into both application and access logs managers

… of the access log descriptors

istio-testing · 2017-04-04T22:29:32Z

Jenkins job mixer/presubmit passed

istio-testing · 2017-04-04T22:38:16Z

Jenkins job istio/mixer-pr-master passed

douglas-reid · 2017-04-04T21:53:25Z

-	}
+	// validation ensures both that the descriptor exists and that its template is parsable by the template library.
+	desc := df.GetLog(cfg.Log.DescriptorName)
+	tmpl, _ := template.New("accessLogsTemplate").Parse(desc.LogTemplate)


worth commenting that this error will have been caught in validation?

The comment above desc two lines up covers both.

doh! reading comprehension fail.

douglas-reid · 2017-04-05T17:19:42Z

+      logName: access_log
      log:
-        logFormat: 1 # COMMON
-        templateExpressions:


can we keep the labels? i still think we want the structured logs, if possible -- even if the config is ugly as sin.

I'll add them back

I see the descriptors now... do we need the labels section here too?

It's there, the github diff doesn't render it cause it's unchanged from before. If you expand the file down you should see the labels.

Oh and make sure you're looking at the latest pushed commit; if you click on the file name here it goes to the commit that you commented on, not the latest thing I pushed (tricked me at first too).

…ng error in the errors returned by common validation code in common.go

ZackButcher · 2017-04-05T18:29:47Z

PTAL

istio-testing · 2017-04-05T18:32:49Z

Jenkins job mixer/presubmit passed

istio-testing · 2017-04-05T18:41:21Z

Jenkins job istio/mixer-pr-master passed

douglas-reid

LGTM. My concerns have been addressed.

I think this highlights some issues with config as it exists currently, but this seems to preserve behavior while making things more explicit and configurable.

ZackButcher requested review from douglas-reid, geeknoid and mandarjog March 31, 2017 21:09

googlebot added the cla: yes label Mar 31, 2017

ZackButcher mentioned this pull request Mar 31, 2017

Update mixer configs to support access logs istio/istio#101

Closed

mandarjog reviewed Mar 31, 2017

View reviewed changes

ZackButcher force-pushed the accesslogs branch from e83295d to 1d99776 Compare April 3, 2017 22:21

ZackButcher added 5 commits April 4, 2017 15:16

Update validation to ensure that template expressions are syntactical…

b0c7645

…ly valid and that the attributes exist in the system to satisfy their expressions. Wire this into both application and access logs managers

add back api.{name, method} attributes as a separate PR removes them

fb09a11

Be consistent on snake vs camel case in config yaml; update the names…

e087967

… of the access log descriptors

Correct things that changed due to enabling gogoproto features.

31ce43f

ZackButcher force-pushed the accesslogs branch from 5d061e9 to 31ce43f Compare April 4, 2017 22:18

douglas-reid reviewed Apr 5, 2017

View reviewed changes

Add labels back to the descriptors for access logs. Fix a format stri…

01c75af

…ng error in the errors returned by common validation code in common.go

douglas-reid approved these changes Apr 6, 2017

View reviewed changes

ZackButcher merged commit 3eb624c into istio:master Apr 6, 2017

ZackButcher deleted the accesslogs branch April 6, 2017 17:49

Conversation

ZackButcher commented Mar 31, 2017 • edited by mandarjog Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-testing commented Mar 31, 2017

Uh oh!

istio-testing commented Mar 31, 2017

Uh oh!

istio-testing commented Mar 31, 2017

Uh oh!

istio-testing commented Mar 31, 2017

Uh oh!

istio-testing commented Mar 31, 2017

Uh oh!

codecov Bot commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

istio-testing commented Mar 31, 2017

Uh oh!

istio-testing commented Mar 31, 2017

Uh oh!

ZackButcher commented Mar 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZackButcher commented Mar 31, 2017

Uh oh!

istio-testing commented Mar 31, 2017

Uh oh!

istio-testing commented Mar 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

istio-testing commented Apr 3, 2017

Uh oh!

istio-testing commented Apr 3, 2017

Uh oh!

ZackButcher commented Apr 3, 2017

Uh oh!

istio-testing commented Apr 4, 2017

Uh oh!

istio-testing commented Apr 4, 2017

Uh oh!

istio-testing commented Apr 4, 2017

Uh oh!

istio-testing commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZackButcher commented Apr 5, 2017

Uh oh!

istio-testing commented Apr 5, 2017

Uh oh!

istio-testing commented Apr 5, 2017

Uh oh!

douglas-reid left a comment

Choose a reason for hiding this comment

ZackButcher commented Mar 31, 2017 •

edited by mandarjog

Loading

codecov Bot commented Mar 31, 2017 •

edited

Loading