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

Robust Attribute Tracing Support #75

Closed

Conversation

@clintoncwolfe
Copy link

@clintoncwolfe clintoncwolfe commented Dec 3, 2014

No description provided.


There are some spec tests included in https://github.com/opscode/chef/pull/1373, but
that may not be what people want; and it does not address the existence of an
real Observable interface.
Copy link
Contributor

@lamont-granquist lamont-granquist Dec 4, 2014

I think I'm mostly concerned that the existing Node::Attribute implementation is turning into a mess, and the Chef 12 changes make for even more entry points to mutate things, and I'm not wild about the way everything is being hacked up with additional state coming along for the ride on the VividMash's and stuff. You found stuff like the difference between the AttrArray and the VividMashes when it comes to auto-promotion of the types and a lot of other edge cases like that. The caching stuff I bolted into Chef-12 to try to make it a bit faster are particularly ugly. So there's that at the lowest level. I'm not even super worried about having a proper Observable interface at this point, but the whole rest of the attribute implementation is lurching along and showing its age. I think I've got a design for how to do it all much better, though, and I think attribute tracing would slot on top of it pretty nicely and we could wrap all the mutators fairly easily.

Getting the information injected correctly is a bit more problematic. The use of class variables and global state will cause issues and that should get moved to the run_context or a proper Singleton class somewhere. Then I'm just not super wild about wrapping all the things that might set a node attribute in Chef::Node::Attribute.with_tracer_hint calls (but I don't have a better solution).

So I think to begin with it'd be nice to see a cleaner implementation of how Chef::Node and friends would be able to ask for the current 'context'. Ideally this goes into something like Chef::ExecutionTracer which is a Singleton with state on the current cookbook, file and such, and which Chef::Node can ask for the current cookbook, file and line number. We also may be able to reuse that code in the error inspectors as well. That would let you do a new feature as a new object, pretty much clean room, would have good separation of concerns and would be reusable outside of attributes.

Should probably come up with a better name than Chef::ExecutionTracker tho....

@nathenharvey
Copy link
Contributor

@nathenharvey nathenharvey commented Mar 13, 2015

should we close this for now and revisit after Attributes API 2 is done?

@lamont-granquist
Copy link
Contributor

@lamont-granquist lamont-granquist commented Mar 13, 2015

yeah, we also need Attributes Core 2 as well

@clintoncwolfe
Copy link
Author

@clintoncwolfe clintoncwolfe commented Mar 13, 2015

WFM
On Mar 12, 2015 11:07 PM, "Lamont Granquist" notifications@github.com
wrote:

yeah, we also need Attributes Core 2 as well


Reply to this email directly or view it on GitHub
#75 (comment).

@nathenharvey
Copy link
Contributor

@nathenharvey nathenharvey commented Mar 13, 2015

Closing for now. Will revisit or incorporate into the new attributes work that's being proposed.

@vinyar
Copy link

@vinyar vinyar commented Apr 14, 2015

👍 for whenever this gets reopened

@odcinek
Copy link

@odcinek odcinek commented Jul 24, 2015

👍 on this, would be great to revive chef/chef#1373

@lamont-granquist
Copy link
Contributor

@lamont-granquist lamont-granquist commented Jul 25, 2015

I need to ship the node attributes re-write when I get back off of vacation. Attribute tracing should get easier after I do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants